Commit b2c46181 authored by Jussi Maki's avatar Jussi Maki Committed by Daniel Borkmann
Browse files

bpf, sockmap: sk_skb data_end access incorrect when src_reg = dst_reg 



The current conversion of skb->data_end reads like this:

  ; data_end = (void*)(long)skb->data_end;
   559: (79) r1 = *(u64 *)(r2 +200)   ; r1  = skb->data
   560: (61) r11 = *(u32 *)(r2 +112)  ; r11 = skb->len
   561: (0f) r1 += r11
   562: (61) r11 = *(u32 *)(r2 +116)
   563: (1f) r1 -= r11

But similar to the case in 84f44df6 ("bpf: sock_ops sk access may stomp
registers when dst_reg = src_reg"), the code will read an incorrect skb->len
when src == dst. In this case we end up generating this xlated code:

  ; data_end = (void*)(long)skb->data_end;
   559: (79) r1 = *(u64 *)(r1 +200)   ; r1  = skb->data
   560: (61) r11 = *(u32 *)(r1 +112)  ; r11 = (skb->data)->len
   561: (0f) r1 += r11
   562: (61) r11 = *(u32 *)(r1 +116)
   563: (1f) r1 -= r11

... where line 560 is the reading 4B of (skb->data + 112) instead of the
intended skb->len Here the skb pointer in r1 gets set to skb->data and the
later deref for skb->len ends up following skb->data instead of skb.

This fixes the issue similarly to the patch mentioned above by creating an
additional temporary variable and using to store the register when dst_reg =
src_reg. We name the variable bpf_temp_reg and place it in the cb context for
sk_skb. Then we restore from the temp to ensure nothing is lost.

Fixes: 16137b09 ("bpf: Compute data_end dynamically with JIT code")
Signed-off-by: default avatarJussi Maki <joamaki@gmail.com>
Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Reviewed-by: default avatarJakub Sitnicki <jakub@cloudflare.com>
Link: https://lore.kernel.org/bpf/20211103204736.248403-6-john.fastabend@gmail.com
parent e0dc3b93

include/net/strparser.h

+4 −0
Original line number Diff line number Diff line
@@ -66,6 +66,10 @@ struct sk_skb_cb { 
#define SK_SKB_CB_PRIV_LEN 20

	unsigned char data[SK_SKB_CB_PRIV_LEN];

	struct _strp_msg strp;

	/* temp_reg is a temporary register used for bpf_convert_data_end_access

	 * when dst_reg == src_reg.

	 */

	u64 temp_reg;

};



static inline struct strp_msg *strp_msg(struct sk_buff *skb)

net/core/filter.c

+30 −6
Original line number Diff line number Diff line
@@ -9756,22 +9756,46 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type, 
static struct bpf_insn *bpf_convert_data_end_access(const struct bpf_insn *si,

						    struct bpf_insn *insn)

{

	/* si->dst_reg = skb->data */

	int reg;

	int temp_reg_off = offsetof(struct sk_buff, cb) +

			   offsetof(struct sk_skb_cb, temp_reg);



	if (si->src_reg == si->dst_reg) {

		/* We need an extra register, choose and save a register. */

		reg = BPF_REG_9;

		if (si->src_reg == reg || si->dst_reg == reg)

			reg--;

		if (si->src_reg == reg || si->dst_reg == reg)

			reg--;

		*insn++ = BPF_STX_MEM(BPF_DW, si->src_reg, reg, temp_reg_off);

	} else {

		reg = si->dst_reg;

	}



	/* reg = skb->data */

	*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data),

			      si->dst_reg, si->src_reg,

			      reg, si->src_reg,

			      offsetof(struct sk_buff, data));

	/* AX = skb->len */

	*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, len),

			      BPF_REG_AX, si->src_reg,

			      offsetof(struct sk_buff, len));

	/* si->dst_reg = skb->data + skb->len */

	*insn++ = BPF_ALU64_REG(BPF_ADD, si->dst_reg, BPF_REG_AX);

	/* reg = skb->data + skb->len */

	*insn++ = BPF_ALU64_REG(BPF_ADD, reg, BPF_REG_AX);

	/* AX = skb->data_len */

	*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, data_len),

			      BPF_REG_AX, si->src_reg,

			      offsetof(struct sk_buff, data_len));

	/* si->dst_reg = skb->data + skb->len - skb->data_len */

	*insn++ = BPF_ALU64_REG(BPF_SUB, si->dst_reg, BPF_REG_AX);



	/* reg = skb->data + skb->len - skb->data_len */

	*insn++ = BPF_ALU64_REG(BPF_SUB, reg, BPF_REG_AX);



	if (si->src_reg == si->dst_reg) {

		/* Restore the saved register */

		*insn++ = BPF_MOV64_REG(BPF_REG_AX, si->src_reg);

		*insn++ = BPF_MOV64_REG(si->dst_reg, reg);

		*insn++ = BPF_LDX_MEM(BPF_DW, reg, BPF_REG_AX, temp_reg_off);

	}



	return insn;

}