Re: [PATCH bpf v1 3/4] bpf: Augment raw_tp arguments with PTR_MAYBE_NULL

[Jiri Olsa <olsajiri@gmail.com>]

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
>