On Thu, Oct 31, 2024 at 5:00 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > > Reported-by: Juri Lelli <juri.lelli@xxxxxxxxxx> > Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs") > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > --- > 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; > } [...]
