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? > return -EACCES; > } > + unmask_raw_tp_reg(reg, mask); > > if (reg->ref_obj_id) { > if (is_kfunc_release(meta) && meta->ref_obj_id) { > @@ -12128,16 +12175,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); same as above, do we care about unmasking in this situation? and the one immediately below? > 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: > @@ -12160,7 +12215,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; > > @@ -12337,6 +12394,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))) && > @@ -12345,9 +12403,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); ditto > 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; > @@ -13320,7 +13380,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; > @@ -13334,6 +13394,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]; > @@ -13360,11 +13421,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); ditto > return -EACCES; > } > + unmask_raw_tp_reg(ptr_reg, mask); > > switch (base_type(ptr_reg->type)) { > case PTR_TO_CTX: > @@ -19866,6 +19930,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; > -- > 2.43.5 >
