On Tue, Dec 10, 2024 at 06:01:55PM -0800, Kumar Kartikeya Dwivedi 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.
>
> A previous attempt [1], i.e. the second fixed commit, was made to
> simulate symbolic execution as if in most accesses, the argument is a
> non-NULL raw_tp, except for conditional jumps. This tried to suppress
> branch prediction while preserving compatibility, but surfaced issues
> with production programs that were difficult to solve without increasing
> verifier complexity. A more complete discussion of issues and fixes is
> available at [2].
>
> Fix this by maintaining an explicit, incomplete list of tracepoints
> where the arguments are known to be NULL, and mark the positional
> arguments as PTR_MAYBE_NULL. Additionally, capture the tracepoints where
> arguments are known to be PTR_ERR, and mark these arguments as scalar
> values to prevent potential dereference.
>
> In the future, an automated pass will be used to produce such a list, or
> insert __nullable annotations automatically for tracepoints. Anyhow,
> this is an attempt to close the gap until the automation lands, and
so this won't cover modules with raw tracepoints, but I guess it's fine
as temporary solution until we have __nullable annotation support
SNIP
> bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const struct bpf_prog *prog,
> struct bpf_insn_access_aux *info)
> @@ -6449,6 +6539,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> const char *tname = prog->aux->attach_func_name;
> struct bpf_verifier_log *log = info->log;
> const struct btf_param *args;
> + bool ptr_err_raw_tp = false;
> const char *tag_value;
> u32 nr_args, arg;
> int i, ret;
> @@ -6591,6 +6682,36 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
> info->reg_type |= PTR_MAYBE_NULL;
>
> + if (prog->expected_attach_type == BPF_TRACE_RAW_TP) {
> + struct btf *btf = prog->aux->attach_btf;
> + const struct btf_type *t;
> + const char *tname;
> +
> + t = btf_type_by_id(btf, prog->aux->attach_btf_id);
> + if (!t)
> + goto done;
> + tname = btf_name_by_offset(btf, t->name_off);
> + if (!tname)
> + goto done;
I think both btf_type_by_id and btf_name_by_offset should succeed for
BPF_TRACE_RAW_TP .. should be already checked in bpf_check_attach_target
> + for (int i = 0; i < ARRAY_SIZE(raw_tp_null_args); i++) {
> + /* Is this a func with potential NULL args? */
> + if (strcmp(tname, raw_tp_null_args[i].func))
> + continue;
> + /* Is the current arg NULL? */
> + if (raw_tp_null_args[i].mask & NULL_ARG(arg + 1))
> + info->reg_type |= PTR_MAYBE_NULL;
> + break;
> + }
> + /* Hardcode the only cases which has a IS_ERR pointer, i.e.
> + * mr_integ_alloc's 4th argument (mr), and
> + * cachefiles_lookup's 3rd argument (de).
> + */
> + if (!strcmp(tname, "btf_trace_mr_integ_alloc") && (arg + 1) == 4)
> + ptr_err_raw_tp = true;
> + if (!strcmp(tname, "btf_trace_cachefiles_lookup") && (arg + 1) == 3)
> + ptr_err_raw_tp = true;
could we have extra mask value (or split the current one in half) in
struct bpf_raw_tp_null_args and use it for scalar arguments? so we don't
have special checks and handle everything in the loop above
jirka
> + }
> +done:
> if (tgt_prog) {
> enum bpf_prog_type tgt_type;
>
> @@ -6635,6 +6756,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> bpf_log(log, "func '%s' arg%d has btf_id %d type %s '%s'\n",
> tname, arg, info->btf_id, btf_type_str(t),
> __btf_name_by_offset(btf, t->name_off));
> +
> + /* Perform all checks on the validity of type for this argument, but if
> + * we know it can be IS_ERR at runtime, scrub pointer type and mark as
> + * scalar. We do not handle is_retval case as we hardcode ptr_err_raw_tp
> + * handling for known tps.
> + */
> + if (ptr_err_raw_tp)
> + info->reg_type = SCALAR_VALUE;
> return true;
> }
> EXPORT_SYMBOL_GPL(btf_ctx_access);
> --
> 2.43.5
>
