Commit 28e33f9d authored Oct 16, 2017 by Jakub Kicinski Committed by David S. Miller Oct 18, 2017

bpf: disallow arithmetic operations on context pointer



Commit f1174f77 ("bpf/verifier: rework value tracking")
removed the crafty selection of which pointer types are
allowed to be modified.  This is OK for most pointer types
since adjust_ptr_min_max_vals() will catch operations on
immutable pointers.  One exception is PTR_TO_CTX which is
now allowed to be offseted freely.

The intent of aforementioned commit was to allow context
access via modified registers.  The offset passed to
->is_valid_access() verifier callback has been adjusted
by the value of the variable offset.

What is missing, however, is taking the variable offset
into account when the context register is used.  Or in terms
of the code adding the offset to the value passed to the
->convert_ctx_access() callback.  This leads to the following
eBPF user code:

     r1 += 68
     r0 = *(u32 *)(r1 + 8)
     exit

being translated to this in kernel space:

   0: (07) r1 += 68
   1: (61) r0 = *(u32 *)(r1 +180)
   2: (95) exit

Offset 8 is corresponding to 180 in the kernel, but offset
76 is valid too.  Verifier will "accept" access to offset
68+8=76 but then "convert" access to offset 8 as 180.
Effective access to offset 248 is beyond the kernel context.
(This is a __sk_buff example on a debug-heavy kernel -
packet mark is 8 -> 180, 76 would be data.)

Dereferencing the modified context pointer is not as easy
as dereferencing other types, because we have to translate
the access to reading a field in kernel structures which is
usually at a different offset and often of a different size.
To allow modifying the pointer we would have to make sure
that given eBPF instruction will always access the same
field or the fields accessed are "compatible" in terms of
offset and size...

Disallow dereferencing modified context pointers and add
to selftests the test case described here.

Fixes: f1174f77 ("bpf/verifier: rework value tracking")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Edward Cree <ecree@solarflare.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

parent 48044eb4

kernel/bpf/verifier.c

+6 −2

Original line number	Original line	Diff line number	Diff line
	@@ -1116,7 +1116,12 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
	/* ctx accesses must be at a fixed offset, so that we can		/* ctx accesses must be at a fixed offset, so that we can
	* determine what type of data were returned.		* determine what type of data were returned.
	*/		*/
	if (!tnum_is_const(reg->var_off)) {		if (reg->off) {
			verbose("dereference of modified ctx ptr R%d off=%d+%d, ctx+const is allowed, ctx+const+const is not\n",
			regno, reg->off, off - reg->off);
			return -EACCES;
			}
			if (!tnum_is_const(reg->var_off) \|\| reg->var_off.value) {
	char tn_buf[48];		char tn_buf[48];

	tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);		tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);
	@@ -1124,7 +1129,6 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
	tn_buf, off, size);		tn_buf, off, size);
	return -EACCES;		return -EACCES;
	}		}
	off += reg->var_off.value;
	err = check_ctx_access(env, insn_idx, off, size, t, &reg_type);		err = check_ctx_access(env, insn_idx, off, size, t, &reg_type);
	if (!err && t == BPF_READ && value_regno >= 0) {		if (!err && t == BPF_READ && value_regno >= 0) {
	/* ctx access returns either a scalar, or a		/* ctx access returns either a scalar, or a

tools/testing/selftests/bpf/test_verifier.c

+14 −0

Original line number	Original line	Diff line number	Diff line
	@@ -6645,6 +6645,20 @@ static struct bpf_test tests[] = {
	.errstr = "BPF_END uses reserved fields",		.errstr = "BPF_END uses reserved fields",
	.result = REJECT,		.result = REJECT,
	},		},
			{
			"arithmetic ops make PTR_TO_CTX unusable",
			.insns = {
			BPF_ALU64_IMM(BPF_ADD, BPF_REG_1,
			offsetof(struct __sk_buff, data) -
			offsetof(struct __sk_buff, mark)),
			BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
			offsetof(struct __sk_buff, mark)),
			BPF_EXIT_INSN(),
			},
			.errstr = "dereference of modified ctx ptr R1 off=68+8, ctx+const is allowed, ctx+const+const is not",
			.result = REJECT,
			.prog_type = BPF_PROG_TYPE_SCHED_CLS,
			},
	};		};

	static int probe_filter_length(const struct bpf_insn *fp)		static int probe_filter_length(const struct bpf_insn *fp)

Admin message