On Wed, 6 Nov 2024 at 16:32, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Mon, Nov 4, 2024 at 9:20 AM 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 > > > > Reviewed-by: Jiri Olsa <jolsa@xxxxxxxxxx> > > Reported-by: Juri Lelli <juri.lelli@xxxxxxxxxx> > > Tested-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 | 79 +++++++++++++++++-- > > .../bpf/progs/test_tp_btf_nullable.c | 6 +- > > 4 files changed, 87 insertions(+), 9 deletions(-) > > > > [...] > > > @@ -12065,12 +12109,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); > > Kumar, > > Do we really need this unmask? We are already erroring out, restoring > reg->type is probably not very important at this point? > Hello Andrii, The reason I undid the masking was to ensure if the register type is printed for some reason, it stays consistent and the masking isn't visible, but I guess the verifier state is printed _before_ an instruction is symbolically executed so it's not helping with anything. I can send a follow up to remove the additional unmasking steps. > [...]
