On Sun, 3 Nov 2024 at 12:41, 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(-) > > 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))) { While looking at this again, I'm wondering if this check is too relaxed. Since we're matching on base_type, this will probably end up allowing any PTR_TO_BTF_ID with PTR_MAYBE_NULL in raw_tp progs. So I think I need to narrow it down, unless I'm missing something. Will wait for a while for comments before respinning v3. > err = check_ptr_to_btf_access(env, regs, regno, off, size, t, > value_regno); > [...]
