On Wed, Nov 6, 2024 at 2:58 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > 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. After sleeping on it I think I prefer to keep the code as-is. Removing unmask() in few error path will make it asymmetrical and harder to reason about. Right now it's a straightforward hack mask and unmask. Optional unmask just begs the question... why? Maybe something will print regs in error path... Maybe not now, but tomorrow. So keep it as-is. No need for follow up.
