| Message ID | 20241204024154.21386-3-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fix for raw_tp PTR_MAYBE_NULL unmarking | expand |
On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote: [...] > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ > +SEC("tp_btf/bpf_testmod_test_raw_tp_null") > +__failure > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") This looks like a bug. 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'. > +int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb) > +{ > + asm volatile ( > + "r1 = *(u64 *)(r1 +0); \ > + r2 = r1; \ > + r3 = 0; \ > + r2 += 8; \ > + if r2 == 0 goto jmp2; \ > + r1 = r1; \ > + *(u64 *)(r3 +0) = r3; \ > + jmp2: " > + :: > + : __clobber_all > + ); > + return 0; > +} [...]
On Wed, 4 Dec 2024 at 21:12, Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote: > > [...] > > > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ > > +SEC("tp_btf/bpf_testmod_test_raw_tp_null") > > +__failure > > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") > > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") > > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") > > This looks like a bug. > 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'. > Hmm, yes, it's broken. I am realizing where we do it now will walk r1 first and we'll not see r2 off != 0 until after we mark it already. I guess we need to do the check sooner outside this function in mark_ptr_or_null_regs. There we have the register being operated on, so if off != 0 we don't walk all regs in state. Do you think that should fix this? > > +int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb) > > +{ > > + asm volatile ( > > + "r1 = *(u64 *)(r1 +0); \ > > + r2 = r1; \ > > + r3 = 0; \ > > + r2 += 8; \ > > + if r2 == 0 goto jmp2; \ > > + r1 = r1; \ > > + *(u64 *)(r3 +0) = r3; \ > > + jmp2: " > > + :: > > + : __clobber_all > > + ); > > + return 0; > > +} > > [...]
On Wed, 4 Dec 2024 at 21:19, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, 4 Dec 2024 at 21:12, Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote: > > > > [...] > > > > > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ > > > +SEC("tp_btf/bpf_testmod_test_raw_tp_null") > > > +__failure > > > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") > > > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") > > > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") > > > > This looks like a bug. > > 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'. > > > > Hmm, yes, it's broken. > I am realizing where we do it now will walk r1 first and we'll not see > r2 off != 0 until after we mark it already. > I guess we need to do the check sooner outside this function in > mark_ptr_or_null_regs. > There we have the register being operated on, so if off != 0 we don't > walk all regs in state. What this will do in both cases:: First, avoid walking states when off != 0, and reset id. If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and remove marks for those with off != 0. > > Do you think that should fix this? > > > > +int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb) > > > +{ > > > + asm volatile ( > > > + "r1 = *(u64 *)(r1 +0); \ > > > + r2 = r1; \ > > > + r3 = 0; \ > > > + r2 += 8; \ > > > + if r2 == 0 goto jmp2; \ > > > + r1 = r1; \ > > > + *(u64 *)(r3 +0) = r3; \ > > > + jmp2: " > > > + :: > > > + : __clobber_all > > > + ); > > > + return 0; > > > +} > > > > [...]
On Wed, Dec 4, 2024 at 12:22 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Wed, 4 Dec 2024 at 21:19, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Wed, 4 Dec 2024 at 21:12, Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote: > > > > > > [...] > > > > > > > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ > > > > +SEC("tp_btf/bpf_testmod_test_raw_tp_null") > > > > +__failure > > > > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") > > > > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") > > > > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") > > > > > > This looks like a bug. > > > 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'. > > > > > > > Hmm, yes, it's broken. > > I am realizing where we do it now will walk r1 first and we'll not see > > r2 off != 0 until after we mark it already. > > I guess we need to do the check sooner outside this function in > > mark_ptr_or_null_regs. > > There we have the register being operated on, so if off != 0 we don't > > walk all regs in state. > > What this will do in both cases:: > First, avoid walking states when off != 0, and reset id. > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and > remove marks for those with off != 0. That's getting intrusive. How about we reset id=0 in adjust_ptr_min_max_vals() right after we suppressed "null-check it first" message for raw_tp-s. That will address the issue as well, right?
On Wed, 4 Dec 2024 at 21:40, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Dec 4, 2024 at 12:22 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Wed, 4 Dec 2024 at 21:19, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > On Wed, 4 Dec 2024 at 21:12, Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > > > On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote: > > > > > > > > [...] > > > > > > > > > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ > > > > > +SEC("tp_btf/bpf_testmod_test_raw_tp_null") > > > > > +__failure > > > > > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") > > > > > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") > > > > > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") > > > > > > > > This looks like a bug. > > > > 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'. > > > > > > > > > > Hmm, yes, it's broken. > > > I am realizing where we do it now will walk r1 first and we'll not see > > > r2 off != 0 until after we mark it already. > > > I guess we need to do the check sooner outside this function in > > > mark_ptr_or_null_regs. > > > There we have the register being operated on, so if off != 0 we don't > > > walk all regs in state. > > > > What this will do in both cases:: > > First, avoid walking states when off != 0, and reset id. > > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and > > remove marks for those with off != 0. > > That's getting intrusive. > How about we reset id=0 in adjust_ptr_min_max_vals() > right after we suppressed "null-check it first" message for raw_tp-s. > > That will address the issue as well, right? Yes (minor detail, it needs to be reset to a new id, otherwise we have warn on maybe_null set but !reg->id, but the idea is the same). Let's see what Eduard thinks and then I can give it a go.
On Wed, 2024-12-04 at 21:48 +0100, Kumar Kartikeya Dwivedi wrote:
[...[
(A) ----.
|
v
> > > What this will do in both cases::
> > > First, avoid walking states when off != 0, and reset id.
> > > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and
> > > remove marks for those with off != 0.
(B) ----.
|
v
> > That's getting intrusive.
> > How about we reset id=0 in adjust_ptr_min_max_vals()
> > right after we suppressed "null-check it first" message for raw_tp-s.
> >
> > That will address the issue as well, right?
>
> Yes (minor detail, it needs to be reset to a new id, otherwise we have
> warn on maybe_null set but !reg->id, but the idea is the same).
> Let's see what Eduard thinks and then I can give it a go.
Sorry for delay.
I like what Kumar is proposing in (A) because it could be generalized:
there is no real harm in doing 'r2 = r1; r2 += 8; r1 != 0; ...'
and what Kumar suggests could be used to lift the "null-check it first ..."
restriction.
However, as far as I understand, the plan is to fix this by generating
two entry tracepoint states: one with parameter as null, another with
parameter not-null (all combinations for every parameter).
If that is the plan, what Alexei suggests in (B) is simpler.
On Wed, Dec 4, 2024 at 1:08 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2024-12-04 at 21:48 +0100, Kumar Kartikeya Dwivedi wrote: > > [...[ > > (A) ----. > | > v > > > > What this will do in both cases:: > > > > First, avoid walking states when off != 0, and reset id. > > > > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and > > > > remove marks for those with off != 0. > > (B) ----. > | > v > > > That's getting intrusive. > > > How about we reset id=0 in adjust_ptr_min_max_vals() > > > right after we suppressed "null-check it first" message for raw_tp-s. > > > > > > That will address the issue as well, right? > > > > Yes (minor detail, it needs to be reset to a new id, otherwise we have > > warn on maybe_null set but !reg->id, but the idea is the same). > > Let's see what Eduard thinks and then I can give it a go. > > Sorry for delay. > > I like what Kumar is proposing in (A) because it could be generalized: > there is no real harm in doing 'r2 = r1; r2 += 8; r1 != 0; ...' > and what Kumar suggests could be used to lift the "null-check it first ..." > restriction. I don't see how it can be generalized. Also 'avoid walking states when off != 0' is far from simple. We call into mark_ptr_or_null_regs() with id == 0 already and with reg->off != 0 for RCU and alloc_obj. 'avoid walking with off != 0' doesn't look trivial. It would need to be special cased to raw_tp and some other conditions. I could be missing something. Let's see how patches look. > However, as far as I understand, the plan is to fix this by generating > two entry tracepoint states: one with parameter as null, another with > parameter not-null (all combinations for every parameter). > If that is the plan, what Alexei suggests in (B) is simpler.
On Wed, 2024-12-04 at 13:13 -0800, Alexei Starovoitov wrote: > On Wed, Dec 4, 2024 at 1:08 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > On Wed, 2024-12-04 at 21:48 +0100, Kumar Kartikeya Dwivedi wrote: > > > > [...[ > > > > (A) ----. > > | > > v > > > > > What this will do in both cases:: > > > > > First, avoid walking states when off != 0, and reset id. > > > > > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and > > > > > remove marks for those with off != 0. > > > > (B) ----. > > | > > v > > > > That's getting intrusive. > > > > How about we reset id=0 in adjust_ptr_min_max_vals() > > > > right after we suppressed "null-check it first" message for raw_tp-s. > > > > > > > > That will address the issue as well, right? > > > > > > Yes (minor detail, it needs to be reset to a new id, otherwise we have > > > warn on maybe_null set but !reg->id, but the idea is the same). > > > Let's see what Eduard thinks and then I can give it a go. > > > > Sorry for delay. > > > > I like what Kumar is proposing in (A) because it could be generalized: > > there is no real harm in doing 'r2 = r1; r2 += 8; r1 != 0; ...' > > and what Kumar suggests could be used to lift the "null-check it first ..." > > restriction. > > I don't see how it can be generalized. > Also 'avoid walking states when off != 0' is far from simple. > We call into mark_ptr_or_null_regs() with id == 0 already > and with reg->off != 0 for RCU and alloc_obj. I did not try to implement this, so there might be a devil in the details. The naive approach looks as below. Suppose we want to allow 'rX += K' when rX is PTR_MAYBE_NULL. Such operations generate a set of pointers with different .off values but same .id . For a regular (non raw_tp) case: - dereferencing PTR_MAYBE_NULL is disallowed; - if there is a check 'if rY != 0' and rY.off == 0, the non-null status could be propagated to each register in a set (and PTR_MAYBE_NULL mark removed); - if there is a check 'if rY != 0' and rY.off != 0, nothing happens, no marks are changed. For a raw_tp case: - dereferencing PTR_MAYBE_NULL is allowed (as it is already); - the mechanics for 'if rY != 0' and rY.off ==/!= 0 can remain the same, nothing is wrong with removing PTR_MAYBE_NULL marks from such pointers. > 'avoid walking with off != 0' doesn't look trivial. > It would need to be special cased to raw_tp and some other > conditions. > I could be missing something. > > Let's see how patches look. > > > However, as far as I understand, the plan is to fix this by generating > > two entry tracepoint states: one with parameter as null, another with > > parameter not-null (all combinations for every parameter). > > If that is the plan, what Alexei suggests in (B) is simpler.
On Wed, 4 Dec 2024 at 22:37, Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Wed, 2024-12-04 at 13:13 -0800, Alexei Starovoitov wrote: > > On Wed, Dec 4, 2024 at 1:08 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > > > > > On Wed, 2024-12-04 at 21:48 +0100, Kumar Kartikeya Dwivedi wrote: > > > > > > [...[ > > > > > > (A) ----. > > > | > > > v > > > > > > What this will do in both cases:: > > > > > > First, avoid walking states when off != 0, and reset id. > > > > > > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and > > > > > > remove marks for those with off != 0. > > > > > > (B) ----. > > > | > > > v > > > > > That's getting intrusive. > > > > > How about we reset id=0 in adjust_ptr_min_max_vals() > > > > > right after we suppressed "null-check it first" message for raw_tp-s. > > > > > > > > > > That will address the issue as well, right? > > > > > > > > Yes (minor detail, it needs to be reset to a new id, otherwise we have > > > > warn on maybe_null set but !reg->id, but the idea is the same). > > > > Let's see what Eduard thinks and then I can give it a go. > > > > > > Sorry for delay. > > > > > > I like what Kumar is proposing in (A) because it could be generalized: > > > there is no real harm in doing 'r2 = r1; r2 += 8; r1 != 0; ...' > > > and what Kumar suggests could be used to lift the "null-check it first ..." > > > restriction. > > > > I don't see how it can be generalized. > > Also 'avoid walking states when off != 0' is far from simple. > > We call into mark_ptr_or_null_regs() with id == 0 already > > and with reg->off != 0 for RCU and alloc_obj. > > I did not try to implement this, so there might be a devil in the details. > The naive approach looks as below. > > Suppose we want to allow 'rX += K' when rX is PTR_MAYBE_NULL. > Such operations generate a set of pointers with different .off values > but same .id . > For a regular (non raw_tp) case: > - dereferencing PTR_MAYBE_NULL is disallowed; > - if there is a check 'if rY != 0' and rY.off == 0, > the non-null status could be propagated to each > register in a set (and PTR_MAYBE_NULL mark removed); > - if there is a check 'if rY != 0' and rY.off != 0, > nothing happens, no marks are changed. Yes, also I realized after some thinking that when rY with off != 0 is checked, it just needs to be a no-op (in context of this solution), we don't need to remove it from the set. Later if rX with off == 0 sharing the same id is checked rY should be marked not null. > > For a raw_tp case: > - dereferencing PTR_MAYBE_NULL is allowed (as it is already); > - the mechanics for 'if rY != 0' and rY.off ==/!= 0 can remain the same, > nothing is wrong with removing PTR_MAYBE_NULL marks from such pointers. Yes, it can be generalized, this solution to generalize to all types does not require the state forking approach, which is different. It is getting late for me here so I will continue looking at this tomorrow, but I can do this for raw_tp in this patch as a fix for the warning. Then, I can send a follow up doing it for all pointer types against bpf-next where can continue discussion based on concrete code. Alexei said he was giving the state forking a go in parallel (which is much more involved and impact on veristat needs to be analyzed). Anyhow, I will continue tomorrow. Let me know what you think. > > > 'avoid walking with off != 0' doesn't look trivial. > > It would need to be special cased to raw_tp and some other > > conditions. > > I could be missing something. > > > > Let's see how patches look. > > > > > However, as far as I understand, the plan is to fix this by generating > > > two entry tracepoint states: one with parameter as null, another with > > > parameter not-null (all combinations for every parameter). > > > If that is the plan, what Alexei suggests in (B) is simpler. >
diff --git a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c index 6fa19449297e..13fcd4c31034 100644 --- a/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c +++ b/tools/testing/selftests/bpf/prog_tests/raw_tp_null.c @@ -3,6 +3,12 @@ #include <test_progs.h> #include "raw_tp_null.skel.h" +#include "raw_tp_null_fail.skel.h" + +void test_raw_tp_null_fail(void) +{ + RUN_TESTS(raw_tp_null_fail); +} void test_raw_tp_null(void) { diff --git a/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c new file mode 100644 index 000000000000..12096150a48c --- /dev/null +++ b/tools/testing/selftests/bpf/progs/raw_tp_null_fail.c @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ + +#include <vmlinux.h> +#include <bpf/bpf_tracing.h> +#include "bpf_misc.h" + +/* r1 with off = 0 is checked, which marks new id for r0 with off=8 */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__failure +__msg("2: (b7) r2 = 0 ; R2_w=0") +__msg("3: (07) r0 += 8 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("4: (15) if r1 == 0x0 goto pc+2 ; R1_w=trusted_ptr_sk_buff()") +__msg("5: (bf) r2 = r0 ; R0_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") +int BPF_PROG(test_raw_tp_null_check_zero_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r0 = r1; \ + r2 = 0; \ + r0 += 8; \ + if r1 == 0 goto jmp; \ + r2 = r0; \ + *(u64 *)(r2 +0) = r2; \ + jmp: " + :: + : __clobber_all + ); + return 0; +} + +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__failure +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()") +int BPF_PROG(test_raw_tp_null_copy_check_with_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r2 = r1; \ + r3 = 0; \ + r2 += 8; \ + if r2 == 0 goto jmp2; \ + r1 = r1; \ + *(u64 *)(r3 +0) = r3; \ + jmp2: " + :: + : __clobber_all + ); + return 0; +} + +/* Ensure id's are incremented everytime things are checked.. */ +SEC("tp_btf/bpf_testmod_test_raw_tp_null") +__failure +__msg("2: (07) r0 += 8 ; R0_w=trusted_ptr_or_null_sk_buff(id=1,off=8)") +__msg("3: (15) if r0 == 0x0 goto pc+4 ; R0_w=trusted_ptr_or_null_sk_buff(id=2,off=8)") +__msg("4: (15) if r0 == 0x0 goto pc+3 ; R0_w=trusted_ptr_or_null_sk_buff(id=4,off=8)") +__msg("5: (15) if r0 == 0x0 goto pc+2 ; R0_w=trusted_ptr_or_null_sk_buff(id=6,off=8)") +__msg("6: (bf) r2 = r0 ; R0_w=trusted_ptr_or_null_sk_buff(id=6,off=8)") +int BPF_PROG(test_raw_tp_check_with_off, struct sk_buff *skb) +{ + asm volatile ( + "r1 = *(u64 *)(r1 +0); \ + r0 = r1; \ + r0 += 8; \ + if r0 == 0 goto jmp3; \ + if r0 == 0 goto jmp3; \ + if r0 == 0 goto jmp3; \ + r2 = r0; \ + *(u64 *)(r2 +0) = r2; \ + jmp3: " + :: + : __clobber_all + ); + return 0; +} + +char _license[] SEC("license") = "GPL";
Ensure that pointers with off != 0 are never unmarked as PTR_MAYBE_NULL, pointers that have off == 0 continue getting unmarked, and pointers that don't get unmarked acquire a new id instead of resetting it to zero, so as to identify themselves uniquely and not hit other warnings where id == 0 is not expected with PTR_MAYBE_NULL. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- .../selftests/bpf/prog_tests/raw_tp_null.c | 6 ++ .../selftests/bpf/progs/raw_tp_null_fail.c | 81 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/raw_tp_null_fail.c