| Message ID | 20241101000017.3424165-2-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Handle possible NULL trusted raw_tp arguments | expand |
On Thu, Oct 31, 2024 at 5:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Arguments to a raw tracepoint are tagged as trusted, which carries the > semantics that the pointer will be non-NULL. However, in certain cases, > a raw tracepoint argument may end up being NULL More context about this > issue is available in [0]. > > Thus, there is a discrepancy between the reality, that raw_tp arguments > can actually be NULL, and the verifier's knowledge, that they are never > NULL, causing explicit NULL checks to be deleted, and accesses to such > pointers potentially crashing the kernel. > > To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special > case the dereference and pointer arithmetic to permit it, and allow > passing them into helpers/kfuncs; these exceptions are made for raw_tp > programs only. Ensure that we don't do this when ref_obj_id > 0, as in > that case this is an acquired object and doesn't need such adjustment. > > The reason we do mask_raw_tp_trusted_reg logic is because other will > recheck in places whether the register is a trusted_reg, and then > consider our register as untrusted when detecting the presence of the > PTR_MAYBE_NULL flag. > > To allow safe dereference, we enable PROBE_MEM marking when we see loads > into trusted pointers with PTR_MAYBE_NULL. > > While trusted raw_tp arguments can also be passed into helpers or kfuncs > where such broken assumption may cause issues, a future patch set will > tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can > already be passed into helpers and causes similar problems. Thus, they > are left alone for now. > > It is possible that these checks also permit passing non-raw_tp args > that are trusted PTR_TO_BTF_ID with null marking. In such a case, > allowing dereference when pointer is NULL expands allowed behavior, so > won't regress existing programs, and the case of passing these into > helpers is the same as above and will be dealt with later. > > Also update the failure case in tp_btf_nullable selftest to capture the > new behavior, as the verifier will no longer cause an error when > directly dereference a raw tracepoint argument marked as __nullable. > > [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb > > Reported-by: Juri Lelli <juri.lelli@redhat.com> > Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs") > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > include/linux/bpf.h | 6 ++ > kernel/bpf/btf.c | 5 +- > kernel/bpf/verifier.c | 75 +++++++++++++++++-- > .../bpf/progs/test_tp_btf_nullable.c | 6 +- > 4 files changed, 83 insertions(+), 9 deletions(-) > [...] > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > if (ret < 0) > return ret; > - > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > + * arguments to the raw_tp. Since internal checks in for trusted > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > + * modifier as problematic, mask it out temporarily for the > + * check. Don't apply this to pointers with ref_obj_id > 0, as > + * those won't be raw_tp args. > + * > + * We may end up applying this relaxation to other trusted > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > + * tagging, but that should expand allowed behavior, and not > + * cause regression for existing behavior. > + */ Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong with the same behavior for BPF iterator programs, for example? It seems nicer if we can avoid this temporary masking and instead support this as a generic functionality? Or are there complications? > + mask = mask_raw_tp_reg(env, reg); > if (ret != PTR_TO_BTF_ID) { > /* just mark; */ > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > clear_trusted_flags(&flag); > } > > - if (atype == BPF_READ && value_regno >= 0) > + if (atype == BPF_READ && value_regno >= 0) { > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > + /* We've assigned a new type to regno, so don't undo masking. */ > + if (regno == value_regno) > + mask = false; > + } > + unmask_raw_tp_reg(reg, mask); > > return 0; > } [...]
On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > if (ret < 0) > > return ret; > > - > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > + * arguments to the raw_tp. Since internal checks in for trusted > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > + * modifier as problematic, mask it out temporarily for the > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > + * those won't be raw_tp args. > > + * > > + * We may end up applying this relaxation to other trusted > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > + * tagging, but that should expand allowed behavior, and not > > + * cause regression for existing behavior. > > + */ > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > with the same behavior for BPF iterator programs, for example? > > It seems nicer if we can avoid this temporary masking and instead > support this as a generic functionality? Or are there complications? > > > + mask = mask_raw_tp_reg(env, reg); > > if (ret != PTR_TO_BTF_ID) { > > /* just mark; */ > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > clear_trusted_flags(&flag); > > } > > > > - if (atype == BPF_READ && value_regno >= 0) > > + if (atype == BPF_READ && value_regno >= 0) { > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > + /* We've assigned a new type to regno, so don't undo masking. */ > > + if (regno == value_regno) > > + mask = false; > > + } > > + unmask_raw_tp_reg(reg, mask); Kumar, I chatted with Andrii offline. All other cases of mask/unmask should probably stay raw_tp specific, but it seems we can make this particular case to be generic. Something like the following: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 797cf3ed32e0..bbd4c03460e3 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, */ flag = PTR_UNTRUSTED; + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL)) { + flag |= PTR_MAYBE_NULL; + goto trusted; } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { +trusted: With the idea that trusted_or_null stays that way for all prog types and bpf_iter__task->task deref stays trusted_or_null instead of being downgraded to ptr_to_btf_id without any flags. So progs can do few less != null checks. Need to think it through.
On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > if (ret < 0) > > > return ret; > > > - > > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > > + * arguments to the raw_tp. Since internal checks in for trusted > > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > > + * modifier as problematic, mask it out temporarily for the > > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > > + * those won't be raw_tp args. > > > + * > > > + * We may end up applying this relaxation to other trusted > > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > > + * tagging, but that should expand allowed behavior, and not > > > + * cause regression for existing behavior. > > > + */ > > > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > > with the same behavior for BPF iterator programs, for example? > > > > It seems nicer if we can avoid this temporary masking and instead > > support this as a generic functionality? Or are there complications? > > We _can_ do this for all programs. The thought process here was to leave existing raw_tp programs unbroken if possible if we're marking their arguments as PTR_MAYBE_NULL, since most of them won't be performing any NULL checks at all. > > > + mask = mask_raw_tp_reg(env, reg); > > > if (ret != PTR_TO_BTF_ID) { > > > /* just mark; */ > > > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > clear_trusted_flags(&flag); > > > } > > > > > > - if (atype == BPF_READ && value_regno >= 0) > > > + if (atype == BPF_READ && value_regno >= 0) { > > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > > + /* We've assigned a new type to regno, so don't undo masking. */ > > > + if (regno == value_regno) > > > + mask = false; > > > + } > > > + unmask_raw_tp_reg(reg, mask); > > Kumar, > > I chatted with Andrii offline. All other cases of mask/unmask > should probably stay raw_tp specific, but it seems we can make > this particular case to be generic. > Something like the following: > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 797cf3ed32e0..bbd4c03460e3 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct > bpf_verifier_env *env, > */ > flag = PTR_UNTRUSTED; > > + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | > PTR_MAYBE_NULL)) { > + flag |= PTR_MAYBE_NULL; > + goto trusted; > } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { > +trusted: > > With the idea that trusted_or_null stays that way for all prog > types and bpf_iter__task->task deref stays trusted_or_null > instead of being downgraded to ptr_to_btf_id without any flags. > So progs can do few less != null checks. > Need to think it through. Ok. But don't allow passing such pointers into helpers, right? We do that for raw_tp to preserve compat, but it would just exacerbate the issue if we start doing it everywhere. So it's just that dereferencing a _or_null pointer becomes an ok thing to do? Let me mull over this for a bit. I'm not sure whether not doing the NULL check is better or worse though. On one hand everything will work without checking for NULL, on the other hand people may also assume the verifier isn't complaining because the pointer is valid, and then they read data from the pointer which always ends up being zero, meaning different things for different kinds of fields. Just thinking out loud, but one of the other concerns would be that we're encouraging people not to do these NULL checks, which means a potential page fault penalty everytime that pointer _is_ NULL, instead of a simple branch, which would certainly be a bit expensive. If this becomes the common case, I think the prog execution latency penalty will be big. It is something to consider.
On Sun, 3 Nov 2024 at 10:16, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > > > if (ret < 0) > > > > return ret; > > > > - > > > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > > > + * arguments to the raw_tp. Since internal checks in for trusted > > > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > > > + * modifier as problematic, mask it out temporarily for the > > > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > > > + * those won't be raw_tp args. > > > > + * > > > > + * We may end up applying this relaxation to other trusted > > > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > > > + * tagging, but that should expand allowed behavior, and not > > > > + * cause regression for existing behavior. > > > > + */ > > > > > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > > > with the same behavior for BPF iterator programs, for example? > > > > > > It seems nicer if we can avoid this temporary masking and instead > > > support this as a generic functionality? Or are there complications? > > > > > We _can_ do this for all programs. The thought process here was to > leave existing raw_tp programs unbroken if possible if we're marking > their arguments as PTR_MAYBE_NULL, since most of them won't be > performing any NULL checks at all. > > > > > + mask = mask_raw_tp_reg(env, reg); > > > > if (ret != PTR_TO_BTF_ID) { > > > > /* just mark; */ > > > > > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > clear_trusted_flags(&flag); > > > > } > > > > > > > > - if (atype == BPF_READ && value_regno >= 0) > > > > + if (atype == BPF_READ && value_regno >= 0) { > > > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > > > + /* We've assigned a new type to regno, so don't undo masking. */ > > > > + if (regno == value_regno) > > > > + mask = false; > > > > + } > > > > + unmask_raw_tp_reg(reg, mask); > > > > Kumar, > > > > I chatted with Andrii offline. All other cases of mask/unmask > > should probably stay raw_tp specific, but it seems we can make > > this particular case to be generic. > > Something like the following: > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 797cf3ed32e0..bbd4c03460e3 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct > > bpf_verifier_env *env, > > */ > > flag = PTR_UNTRUSTED; > > > > + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | > > PTR_MAYBE_NULL)) { > > + flag |= PTR_MAYBE_NULL; > > + goto trusted; > > } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { > > +trusted: > > > > With the idea that trusted_or_null stays that way for all prog > > types and bpf_iter__task->task deref stays trusted_or_null > > instead of being downgraded to ptr_to_btf_id without any flags. > > So progs can do few less != null checks. > > Need to think it through. > > Ok. But don't allow passing such pointers into helpers, right? > We do that for raw_tp to preserve compat, but it would just exacerbate > the issue if we start doing it everywhere. > So it's just that dereferencing a _or_null pointer becomes an ok thing to do? > Let me mull over this for a bit. > > I'm not sure whether not doing the NULL check is better or worse > though. On one hand everything will work without checking for NULL, on > the other hand people may also assume the verifier isn't complaining > because the pointer is valid, and then they read data from the pointer > which always ends up being zero, meaning different things for > different kinds of fields. > > Just thinking out loud, but one of the other concerns would be that > we're encouraging people not to do these NULL checks, which means a > potential page fault penalty everytime that pointer _is_ NULL, instead > of a simple branch, which would certainly be a bit expensive. If this > becomes the common case, I think the prog execution latency penalty > will be big. It is something to consider. Ah, no, my bad, this won't be a problem now, as the JIT does emit a branch to check for kernel addresses, but it probably will be if https://lore.kernel.org/bpf/20240619092216.1780946-1-memxor@gmail.com/ gets accepted.
On Sun, 3 Nov 2024 at 10:40, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sun, 3 Nov 2024 at 10:16, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > > > > > if (ret < 0) > > > > > return ret; > > > > > - > > > > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > > > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > > > > + * arguments to the raw_tp. Since internal checks in for trusted > > > > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > > > > + * modifier as problematic, mask it out temporarily for the > > > > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > > > > + * those won't be raw_tp args. > > > > > + * > > > > > + * We may end up applying this relaxation to other trusted > > > > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > > > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > > > > + * tagging, but that should expand allowed behavior, and not > > > > > + * cause regression for existing behavior. > > > > > + */ > > > > > > > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > > > > with the same behavior for BPF iterator programs, for example? > > > > > > > > It seems nicer if we can avoid this temporary masking and instead > > > > support this as a generic functionality? Or are there complications? > > > > > > > > We _can_ do this for all programs. The thought process here was to > > leave existing raw_tp programs unbroken if possible if we're marking > > their arguments as PTR_MAYBE_NULL, since most of them won't be > > performing any NULL checks at all. > > > > > > > + mask = mask_raw_tp_reg(env, reg); > > > > > if (ret != PTR_TO_BTF_ID) { > > > > > /* just mark; */ > > > > > > > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > clear_trusted_flags(&flag); > > > > > } > > > > > > > > > > - if (atype == BPF_READ && value_regno >= 0) > > > > > + if (atype == BPF_READ && value_regno >= 0) { > > > > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > > > > + /* We've assigned a new type to regno, so don't undo masking. */ > > > > > + if (regno == value_regno) > > > > > + mask = false; > > > > > + } > > > > > + unmask_raw_tp_reg(reg, mask); > > > > > > Kumar, > > > > > > I chatted with Andrii offline. All other cases of mask/unmask > > > should probably stay raw_tp specific, but it seems we can make > > > this particular case to be generic. > > > Something like the following: > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 797cf3ed32e0..bbd4c03460e3 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct > > > bpf_verifier_env *env, > > > */ > > > flag = PTR_UNTRUSTED; > > > > > > + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | > > > PTR_MAYBE_NULL)) { > > > + flag |= PTR_MAYBE_NULL; > > > + goto trusted; > > > } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { > > > +trusted: > > > > > > With the idea that trusted_or_null stays that way for all prog > > > types and bpf_iter__task->task deref stays trusted_or_null > > > instead of being downgraded to ptr_to_btf_id without any flags. > > > So progs can do few less != null checks. > > > Need to think it through. > > > > Ok. But don't allow passing such pointers into helpers, right? > > We do that for raw_tp to preserve compat, but it would just exacerbate > > the issue if we start doing it everywhere. > > So it's just that dereferencing a _or_null pointer becomes an ok thing to do? > > Let me mull over this for a bit. > > > > I'm not sure whether not doing the NULL check is better or worse > > though. On one hand everything will work without checking for NULL, on > > the other hand people may also assume the verifier isn't complaining > > because the pointer is valid, and then they read data from the pointer > > which always ends up being zero, meaning different things for > > different kinds of fields. > > > > Just thinking out loud, but one of the other concerns would be that > > we're encouraging people not to do these NULL checks, which means a > > potential page fault penalty everytime that pointer _is_ NULL, instead > > of a simple branch, which would certainly be a bit expensive. If this > > becomes the common case, I think the prog execution latency penalty > > will be big. It is something to consider. > > Ah, no, my bad, this won't be a problem now, as the JIT does emit a > branch to check for kernel addresses, but it probably will be if > https://lore.kernel.org/bpf/20240619092216.1780946-1-memxor@gmail.com/ > gets accepted. I applied this and tried to find out the time it takes to dereference a NULL pointer using bpf_ktime_get_ns With the patch above: time=3345 ns Without (bpf-next right now): time=170 ns So I guess that means I should probably drop the patch above if we decide to allow dereferencing NULL for all programs.
On Sun, Nov 3, 2024 at 9:01 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Sun, 3 Nov 2024 at 10:40, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Sun, 3 Nov 2024 at 10:16, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > - > > > > > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > > > > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > > > > > + * arguments to the raw_tp. Since internal checks in for trusted > > > > > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > > > > > + * modifier as problematic, mask it out temporarily for the > > > > > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > > > > > + * those won't be raw_tp args. > > > > > > + * > > > > > > + * We may end up applying this relaxation to other trusted > > > > > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > > > > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > > > > > + * tagging, but that should expand allowed behavior, and not > > > > > > + * cause regression for existing behavior. > > > > > > + */ > > > > > > > > > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > > > > > with the same behavior for BPF iterator programs, for example? > > > > > > > > > > It seems nicer if we can avoid this temporary masking and instead > > > > > support this as a generic functionality? Or are there complications? > > > > > > > > > > > We _can_ do this for all programs. The thought process here was to > > > leave existing raw_tp programs unbroken if possible if we're marking > > > their arguments as PTR_MAYBE_NULL, since most of them won't be > > > performing any NULL checks at all. > > > > > > > > > + mask = mask_raw_tp_reg(env, reg); > > > > > > if (ret != PTR_TO_BTF_ID) { > > > > > > /* just mark; */ > > > > > > > > > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > clear_trusted_flags(&flag); > > > > > > } > > > > > > > > > > > > - if (atype == BPF_READ && value_regno >= 0) > > > > > > + if (atype == BPF_READ && value_regno >= 0) { > > > > > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > > > > > + /* We've assigned a new type to regno, so don't undo masking. */ > > > > > > + if (regno == value_regno) > > > > > > + mask = false; > > > > > > + } > > > > > > + unmask_raw_tp_reg(reg, mask); > > > > > > > > Kumar, > > > > > > > > I chatted with Andrii offline. All other cases of mask/unmask > > > > should probably stay raw_tp specific, but it seems we can make > > > > this particular case to be generic. > > > > Something like the following: > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index 797cf3ed32e0..bbd4c03460e3 100644 > > > > --- a/kernel/bpf/verifier.c > > > > +++ b/kernel/bpf/verifier.c > > > > @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct > > > > bpf_verifier_env *env, > > > > */ > > > > flag = PTR_UNTRUSTED; > > > > > > > > + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | > > > > PTR_MAYBE_NULL)) { > > > > + flag |= PTR_MAYBE_NULL; > > > > + goto trusted; > > > > } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { > > > > +trusted: > > > > > > > > With the idea that trusted_or_null stays that way for all prog > > > > types and bpf_iter__task->task deref stays trusted_or_null > > > > instead of being downgraded to ptr_to_btf_id without any flags. > > > > So progs can do few less != null checks. > > > > Need to think it through. > > > > > > Ok. But don't allow passing such pointers into helpers, right? > > > We do that for raw_tp to preserve compat, but it would just exacerbate > > > the issue if we start doing it everywhere. > > > So it's just that dereferencing a _or_null pointer becomes an ok thing to do? > > > Let me mull over this for a bit. > > > > > > I'm not sure whether not doing the NULL check is better or worse > > > though. On one hand everything will work without checking for NULL, on > > > the other hand people may also assume the verifier isn't complaining > > > because the pointer is valid, and then they read data from the pointer > > > which always ends up being zero, meaning different things for > > > different kinds of fields. > > > > > > Just thinking out loud, but one of the other concerns would be that > > > we're encouraging people not to do these NULL checks, which means a > > > potential page fault penalty everytime that pointer _is_ NULL, instead > > > of a simple branch, which would certainly be a bit expensive. If this > > > becomes the common case, I think the prog execution latency penalty > > > will be big. It is something to consider. > > > > Ah, no, my bad, this won't be a problem now, as the JIT does emit a > > branch to check for kernel addresses, but it probably will be if > > https://lore.kernel.org/bpf/20240619092216.1780946-1-memxor@gmail.com/ > > gets accepted. > > I applied this and tried to find out the time it takes to dereference > a NULL pointer using bpf_ktime_get_ns > With the patch above: time=3345 ns > Without (bpf-next right now): time=170 ns > > So I guess that means I should probably drop the patch above if we > decide to allow dereferencing NULL for all programs. Your concerns are valid. Accepting deref of trusted_or_null generically will cause these issues long term. It's indeed better to make programmers add explicit !=NULL to their programs. So scratch my earlier suggestion. Let's keep this patch as-is with special hack for raw_tp only that we will hopefully resolve later either with __nullable suffix or compiler detection of nullability. The latter we discussed briefly with Eduard. It's doable to teach llvm to emit btf_tag to aid the verifier. gcc may catch up eventually.
On Sun, Nov 3, 2024 at 9:37 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Sun, Nov 3, 2024 at 9:01 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > On Sun, 3 Nov 2024 at 10:40, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > On Sun, 3 Nov 2024 at 10:16, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > On Fri, 1 Nov 2024 at 17:56, Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Fri, Nov 1, 2024 at 12:16 PM Andrii Nakryiko > > > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > > > > @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > > > > > > > > > if (ret < 0) > > > > > > > return ret; > > > > > > > - > > > > > > > + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL > > > > > > > + * trusted PTR_TO_BTF_ID, these are the ones that are possibly > > > > > > > + * arguments to the raw_tp. Since internal checks in for trusted > > > > > > > + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL > > > > > > > + * modifier as problematic, mask it out temporarily for the > > > > > > > + * check. Don't apply this to pointers with ref_obj_id > 0, as > > > > > > > + * those won't be raw_tp args. > > > > > > > + * > > > > > > > + * We may end up applying this relaxation to other trusted > > > > > > > + * PTR_TO_BTF_ID with maybe null flag, since we cannot > > > > > > > + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal > > > > > > > + * tagging, but that should expand allowed behavior, and not > > > > > > > + * cause regression for existing behavior. > > > > > > > + */ > > > > > > > > > > > > Yeah, I'm not sure why this has to be raw tp-specific?.. What's wrong > > > > > > with the same behavior for BPF iterator programs, for example? > > > > > > > > > > > > It seems nicer if we can avoid this temporary masking and instead > > > > > > support this as a generic functionality? Or are there complications? > > > > > > > > > > > > > > We _can_ do this for all programs. The thought process here was to > > > > leave existing raw_tp programs unbroken if possible if we're marking > > > > their arguments as PTR_MAYBE_NULL, since most of them won't be > > > > performing any NULL checks at all. > > > > > > > > > > > + mask = mask_raw_tp_reg(env, reg); > > > > > > > if (ret != PTR_TO_BTF_ID) { > > > > > > > /* just mark; */ > > > > > > > > > > > > > > @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, > > > > > > > clear_trusted_flags(&flag); > > > > > > > } > > > > > > > > > > > > > > - if (atype == BPF_READ && value_regno >= 0) > > > > > > > + if (atype == BPF_READ && value_regno >= 0) { > > > > > > > mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); > > > > > > > + /* We've assigned a new type to regno, so don't undo masking. */ > > > > > > > + if (regno == value_regno) > > > > > > > + mask = false; > > > > > > > + } > > > > > > > + unmask_raw_tp_reg(reg, mask); > > > > > > > > > > Kumar, > > > > > > > > > > I chatted with Andrii offline. All other cases of mask/unmask > > > > > should probably stay raw_tp specific, but it seems we can make > > > > > this particular case to be generic. > > > > > Something like the following: > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > index 797cf3ed32e0..bbd4c03460e3 100644 > > > > > --- a/kernel/bpf/verifier.c > > > > > +++ b/kernel/bpf/verifier.c > > > > > @@ -6703,7 +6703,11 @@ static int check_ptr_to_btf_access(struct > > > > > bpf_verifier_env *env, > > > > > */ > > > > > flag = PTR_UNTRUSTED; > > > > > > > > > > + } else if (reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | > > > > > PTR_MAYBE_NULL)) { > > > > > + flag |= PTR_MAYBE_NULL; > > > > > + goto trusted; > > > > > } else if (is_trusted_reg(reg) || is_rcu_reg(reg)) { > > > > > +trusted: > > > > > > > > > > With the idea that trusted_or_null stays that way for all prog > > > > > types and bpf_iter__task->task deref stays trusted_or_null > > > > > instead of being downgraded to ptr_to_btf_id without any flags. > > > > > So progs can do few less != null checks. > > > > > Need to think it through. > > > > > > > > Ok. But don't allow passing such pointers into helpers, right? > > > > We do that for raw_tp to preserve compat, but it would just exacerbate > > > > the issue if we start doing it everywhere. > > > > So it's just that dereferencing a _or_null pointer becomes an ok thing to do? > > > > Let me mull over this for a bit. > > > > > > > > I'm not sure whether not doing the NULL check is better or worse > > > > though. On one hand everything will work without checking for NULL, on > > > > the other hand people may also assume the verifier isn't complaining > > > > because the pointer is valid, and then they read data from the pointer > > > > which always ends up being zero, meaning different things for > > > > different kinds of fields. > > > > > > > > Just thinking out loud, but one of the other concerns would be that > > > > we're encouraging people not to do these NULL checks, which means a > > > > potential page fault penalty everytime that pointer _is_ NULL, instead > > > > of a simple branch, which would certainly be a bit expensive. If this > > > > becomes the common case, I think the prog execution latency penalty > > > > will be big. It is something to consider. > > > > > > Ah, no, my bad, this won't be a problem now, as the JIT does emit a > > > branch to check for kernel addresses, but it probably will be if > > > https://lore.kernel.org/bpf/20240619092216.1780946-1-memxor@gmail.com/ > > > gets accepted. > > > > I applied this and tried to find out the time it takes to dereference > > a NULL pointer using bpf_ktime_get_ns > > With the patch above: time=3345 ns > > Without (bpf-next right now): time=170 ns > > > > So I guess that means I should probably drop the patch above if we > > decide to allow dereferencing NULL for all programs. > > Your concerns are valid. > Accepting deref of trusted_or_null generically will cause these issues > long term. It's indeed better to make programmers add explicit !=NULL > to their programs. Agreed. Not a huge fan of mutating reg->type in place with mask/unmask, but it's fine overall. I think we could have removed half of unmasks because they are done in error cases, at which point it doesn't matter, but that's a minor complaint. > So scratch my earlier suggestion. Let's keep this patch as-is with special > hack for raw_tp only that we will hopefully resolve later > either with __nullable suffix or compiler detection of nullability. > The latter we discussed briefly with Eduard. > It's doable to teach llvm to emit btf_tag to aid the verifier. > gcc may catch up eventually.
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index c3ba4d475174..1b84613b10ac 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -3495,4 +3495,10 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog) return prog->aux->func_idx != 0; } +static inline bool bpf_prog_is_raw_tp(const struct bpf_prog *prog) +{ + return prog->type == BPF_PROG_TYPE_TRACING && + prog->expected_attach_type == BPF_TRACE_RAW_TP; +} + #endif /* _LINUX_BPF_H */ diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index ed3219da7181..e7a59e6462a9 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6588,7 +6588,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, if (prog_args_trusted(prog)) info->reg_type |= PTR_TRUSTED; - if (btf_param_match_suffix(btf, &args[arg], "__nullable")) + /* Raw tracepoint arguments always get marked as maybe NULL */ + if (bpf_prog_is_raw_tp(prog)) + info->reg_type |= PTR_MAYBE_NULL; + else if (btf_param_match_suffix(btf, &args[arg], "__nullable")) info->reg_type |= PTR_MAYBE_NULL; if (tgt_prog) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 797cf3ed32e0..36776624710f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -418,6 +418,21 @@ static struct btf_record *reg_btf_record(const struct bpf_reg_state *reg) return rec; } +static bool mask_raw_tp_reg(const struct bpf_verifier_env *env, struct bpf_reg_state *reg) +{ + if (reg->type != (PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL) || + !bpf_prog_is_raw_tp(env->prog) || reg->ref_obj_id) + return false; + reg->type &= ~PTR_MAYBE_NULL; + return true; +} + +static void unmask_raw_tp_reg(struct bpf_reg_state *reg, bool result) +{ + if (result) + reg->type |= PTR_MAYBE_NULL; +} + static bool subprog_is_global(const struct bpf_verifier_env *env, int subprog) { struct bpf_func_info_aux *aux = env->prog->aux->func_info_aux; @@ -6622,6 +6637,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, const char *field_name = NULL; enum bpf_type_flag flag = 0; u32 btf_id = 0; + bool mask; int ret; if (!env->allow_ptr_leaks) { @@ -6693,7 +6709,21 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, if (ret < 0) return ret; - + /* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL + * trusted PTR_TO_BTF_ID, these are the ones that are possibly + * arguments to the raw_tp. Since internal checks in for trusted + * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL + * modifier as problematic, mask it out temporarily for the + * check. Don't apply this to pointers with ref_obj_id > 0, as + * those won't be raw_tp args. + * + * We may end up applying this relaxation to other trusted + * PTR_TO_BTF_ID with maybe null flag, since we cannot + * distinguish PTR_MAYBE_NULL tagged for arguments vs normal + * tagging, but that should expand allowed behavior, and not + * cause regression for existing behavior. + */ + mask = mask_raw_tp_reg(env, reg); if (ret != PTR_TO_BTF_ID) { /* just mark; */ @@ -6754,8 +6784,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, clear_trusted_flags(&flag); } - if (atype == BPF_READ && value_regno >= 0) + if (atype == BPF_READ && value_regno >= 0) { mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag); + /* We've assigned a new type to regno, so don't undo masking. */ + if (regno == value_regno) + mask = false; + } + unmask_raw_tp_reg(reg, mask); return 0; } @@ -7140,7 +7175,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); } else if (base_type(reg->type) == PTR_TO_BTF_ID && - !type_may_be_null(reg->type)) { + (bpf_prog_is_raw_tp(env->prog) || !type_may_be_null(reg->type))) { err = check_ptr_to_btf_access(env, regs, regno, off, size, t, value_regno); } else if (reg->type == CONST_PTR_TO_MAP) { @@ -8833,6 +8868,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, enum bpf_reg_type type = reg->type; u32 *arg_btf_id = NULL; int err = 0; + bool mask; if (arg_type == ARG_DONTCARE) return 0; @@ -8873,11 +8909,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, base_type(arg_type) == ARG_PTR_TO_SPIN_LOCK) arg_btf_id = fn->arg_btf_id[arg]; + mask = mask_raw_tp_reg(env, reg); err = check_reg_type(env, regno, arg_type, arg_btf_id, meta); - if (err) - return err; - err = check_func_arg_reg_off(env, reg, regno, arg_type); + err = err ?: check_func_arg_reg_off(env, reg, regno, arg_type); + unmask_raw_tp_reg(reg, mask); if (err) return err; @@ -9672,14 +9708,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog, return ret; } else if (base_type(arg->arg_type) == ARG_PTR_TO_BTF_ID) { struct bpf_call_arg_meta meta; + bool mask; int err; if (register_is_null(reg) && type_may_be_null(arg->arg_type)) continue; memset(&meta, 0, sizeof(meta)); /* leave func_id as zero */ + mask = mask_raw_tp_reg(env, reg); err = check_reg_type(env, regno, arg->arg_type, &arg->btf_id, &meta); err = err ?: check_func_arg_reg_off(env, reg, regno, arg->arg_type); + unmask_raw_tp_reg(reg, mask); if (err) return err; } else { @@ -11981,6 +12020,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ enum bpf_arg_type arg_type = ARG_DONTCARE; u32 regno = i + 1, ref_id, type_size; bool is_ret_buf_sz = false; + bool mask = false; int kf_arg_type; t = btf_type_skip_modifiers(btf, args[i].type, NULL); @@ -12039,12 +12079,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return -EINVAL; } + mask = mask_raw_tp_reg(env, reg); if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) && (register_is_null(reg) || type_may_be_null(reg->type)) && !is_kfunc_arg_nullable(meta->btf, &args[i])) { verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i); + unmask_raw_tp_reg(reg, mask); return -EACCES; } + unmask_raw_tp_reg(reg, mask); if (reg->ref_obj_id) { if (is_kfunc_release(meta) && meta->ref_obj_id) { @@ -12102,16 +12145,24 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta)) break; + /* Allow passing maybe NULL raw_tp arguments to + * kfuncs for compatibility. Don't apply this to + * arguments with ref_obj_id > 0. + */ + mask = mask_raw_tp_reg(env, reg); if (!is_trusted_reg(reg)) { if (!is_kfunc_rcu(meta)) { verbose(env, "R%d must be referenced or trusted\n", regno); + unmask_raw_tp_reg(reg, mask); return -EINVAL; } if (!is_rcu_reg(reg)) { verbose(env, "R%d must be a rcu pointer\n", regno); + unmask_raw_tp_reg(reg, mask); return -EINVAL; } } + unmask_raw_tp_reg(reg, mask); fallthrough; case KF_ARG_PTR_TO_CTX: case KF_ARG_PTR_TO_DYNPTR: @@ -12134,7 +12185,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ if (is_kfunc_release(meta) && reg->ref_obj_id) arg_type |= OBJ_RELEASE; + mask = mask_raw_tp_reg(env, reg); ret = check_func_arg_reg_off(env, reg, regno, arg_type); + unmask_raw_tp_reg(reg, mask); if (ret < 0) return ret; @@ -12311,6 +12364,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ ref_tname = btf_name_by_offset(btf, ref_t->name_off); fallthrough; case KF_ARG_PTR_TO_BTF_ID: + mask = mask_raw_tp_reg(env, reg); /* Only base_type is checked, further checks are done here */ if ((base_type(reg->type) != PTR_TO_BTF_ID || (bpf_type_has_unsafe_modifiers(reg->type) && !is_rcu_reg(reg))) && @@ -12319,9 +12373,11 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ verbose(env, "expected %s or socket\n", reg_type_str(env, base_type(reg->type) | (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS))); + unmask_raw_tp_reg(reg, mask); return -EINVAL; } ret = process_kf_arg_ptr_to_btf_id(env, reg, ref_t, ref_tname, ref_id, meta, i); + unmask_raw_tp_reg(reg, mask); if (ret < 0) return ret; break; @@ -13294,7 +13350,7 @@ static int sanitize_check_bounds(struct bpf_verifier_env *env, */ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, struct bpf_insn *insn, - const struct bpf_reg_state *ptr_reg, + struct bpf_reg_state *ptr_reg, const struct bpf_reg_state *off_reg) { struct bpf_verifier_state *vstate = env->cur_state; @@ -13308,6 +13364,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, struct bpf_sanitize_info info = {}; u8 opcode = BPF_OP(insn->code); u32 dst = insn->dst_reg; + bool mask; int ret; dst_reg = ®s[dst]; @@ -13334,11 +13391,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env, return -EACCES; } + mask = mask_raw_tp_reg(env, ptr_reg); if (ptr_reg->type & PTR_MAYBE_NULL) { verbose(env, "R%d pointer arithmetic on %s prohibited, null-check it first\n", dst, reg_type_str(env, ptr_reg->type)); + unmask_raw_tp_reg(ptr_reg, mask); return -EACCES; } + unmask_raw_tp_reg(ptr_reg, mask); switch (base_type(ptr_reg->type)) { case PTR_TO_CTX: @@ -19873,6 +19933,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env) * for this case. */ case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED: + case PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL: if (type == BPF_READ) { if (BPF_MODE(insn->code) == BPF_MEM) insn->code = BPF_LDX | BPF_PROBE_MEM | diff --git a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c index bba3e37f749b..5aaf2b065f86 100644 --- a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c +++ b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c @@ -7,7 +7,11 @@ #include "bpf_misc.h" SEC("tp_btf/bpf_testmod_test_nullable_bare") -__failure __msg("R1 invalid mem access 'trusted_ptr_or_null_'") +/* This used to be a failure test, but raw_tp nullable arguments can now + * directly be dereferenced, whether they have nullable annotation or not, + * and don't need to be explicitly checked. + */ +__success int BPF_PROG(handle_tp_btf_nullable_bare1, struct bpf_testmod_test_read_ctx *nullable_ctx) { return nullable_ctx->len;
Arguments to a raw tracepoint are tagged as trusted, which carries the semantics that the pointer will be non-NULL. However, in certain cases, a raw tracepoint argument may end up being NULL More context about this issue is available in [0]. Thus, there is a discrepancy between the reality, that raw_tp arguments can actually be NULL, and the verifier's knowledge, that they are never NULL, causing explicit NULL checks to be deleted, and accesses to such pointers potentially crashing the kernel. To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special case the dereference and pointer arithmetic to permit it, and allow passing them into helpers/kfuncs; these exceptions are made for raw_tp programs only. Ensure that we don't do this when ref_obj_id > 0, as in that case this is an acquired object and doesn't need such adjustment. The reason we do mask_raw_tp_trusted_reg logic is because other will recheck in places whether the register is a trusted_reg, and then consider our register as untrusted when detecting the presence of the PTR_MAYBE_NULL flag. To allow safe dereference, we enable PROBE_MEM marking when we see loads into trusted pointers with PTR_MAYBE_NULL. While trusted raw_tp arguments can also be passed into helpers or kfuncs where such broken assumption may cause issues, a future patch set will tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can already be passed into helpers and causes similar problems. Thus, they are left alone for now. It is possible that these checks also permit passing non-raw_tp args that are trusted PTR_TO_BTF_ID with null marking. In such a case, allowing dereference when pointer is NULL expands allowed behavior, so won't regress existing programs, and the case of passing these into helpers is the same as above and will be dealt with later. Also update the failure case in tp_btf_nullable selftest to capture the new behavior, as the verifier will no longer cause an error when directly dereference a raw tracepoint argument marked as __nullable. [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb Reported-by: Juri Lelli <juri.lelli@redhat.com> Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs") Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- include/linux/bpf.h | 6 ++ kernel/bpf/btf.c | 5 +- kernel/bpf/verifier.c | 75 +++++++++++++++++-- .../bpf/progs/test_tp_btf_nullable.c | 6 +- 4 files changed, 83 insertions(+), 9 deletions(-)