| Message ID | 20171016181655.16366-1-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] bpf: disallow arithmetic operations on context pointer | expand |
On 10/16/2017 08:16 PM, Jakub Kicinski wrote: > Commit f1174f77b50c ("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: f1174f77b50c ("bpf/verifier: rework value tracking") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Thanks for the fix! Acked-by: Daniel Borkmann <daniel@iogearbox.net>
On Mon, Oct 16, 2017 at 11:16:55AM -0700, Jakub Kicinski wrote: > Commit f1174f77b50c ("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: f1174f77b50c ("bpf/verifier: rework value tracking") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Acked-by: Alexei Starovoitov <ast@kernel.org> that's a required fix for net and for net-next I like Ed's idea to teach convert_ctx_access to recognize such complex pattern and do fancier rewrite. iirc we were hitting similar ctx rewrite issue from time to time and managed to change .c and/or llvm codegen in the past when compiler was producing code that technically was valid, but didn't satisfy 'ctx+const' requirement. Back then verifier wasn't that smart. Now we can do better.
On 16/10/17 19:16, Jakub Kicinski wrote: > Commit f1174f77b50c ("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: f1174f77b50c ("bpf/verifier: rework value tracking") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > Dave, a merge note - in net-next this will need env to be passed > to verbose(). > > v2: > - spell dereference correctly. > > kernel/bpf/verifier.c | 8 ++++++-- > tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ > 2 files changed, 20 insertions(+), 2 deletions(-) Acked-by: Edward Cree <ecree@solarflare.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com> Date: Mon, 16 Oct 2017 11:16:55 -0700 > Commit f1174f77b50c ("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: f1174f77b50c ("bpf/verifier: rework value tracking") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Applied. > --- > Dave, a merge note - in net-next this will need env to be passed > to verbose(). Thanks for the note.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 8b8d6ba39e23..20f3889c006e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -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 * 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]; 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); return -EACCES; } - off += reg->var_off.value; err = check_ctx_access(env, insn_idx, off, size, t, ®_type); if (!err && t == BPF_READ && value_regno >= 0) { /* ctx access returns either a scalar, or a diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 26f3250bdcd2..3c7d3a45a3c5 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -6645,6 +6645,20 @@ static struct bpf_test tests[] = { .errstr = "BPF_END uses reserved fields", .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)
Commit f1174f77b50c ("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: f1174f77b50c ("bpf/verifier: rework value tracking") Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- Dave, a merge note - in net-next this will need env to be passed to verbose(). v2: - spell dereference correctly. kernel/bpf/verifier.c | 8 ++++++-- tools/testing/selftests/bpf/test_verifier.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-)