Re: [PATCH bpf v2 2/3] bpf: Augment raw_tp arguments with PTR_MAYBE_NULL

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-12-13 at 09:51 -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 check branch to be dead code eliminated.
> 
> 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 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 ERR_PTR, and mark these arguments as scalar values to
> prevent potential dereference.
> 
> Each hex digit is used to encode NULL-ness (0x1) or ERR_PTR-ness (0x2),
> shifted by the zero-indexed argument number x 4. This can be represented
> as follows:
> 1st arg: 0x1
> 2nd arg: 0x10
> 3rd arg: 0x100
> ... and so on (likewise for ERR_PTR case).
> 
> In the future, an automated pass will be used to produce such a list, or
> insert __nullable annotations automatically for tracepoints. Each
> compilation unit will be analyzed and results will be collated to find
> whether a tracepoint pointer is definitely not null, maybe null, or an
> unknown state where verifier conservatively marks it PTR_MAYBE_NULL.
> A proof of concept of this tool from Eduard is available at [3].
> 
> Note that in case we don't find a specification in the raw_tp_null_args
> array and the tracepoint belongs to a kernel module, we will
> conservatively mark the arguments as PTR_MAYBE_NULL. This is because
> unlike for in-tree modules, out-of-tree module tracepoints may pass NULL
> freely to the tracepoint. We don't protect against such tracepoints
> passing ERR_PTR (which is uncommon anyway), lest we mark all such
> arguments as SCALAR_VALUE.
> 
> While we are it, let's adjust the test raw_tp_null to not perform
> dereference of the skb->mark, as that won't be allowed anymore, and make
> it more robust by using inline assembly to test the dead code
> elimination behavior, which should still stay the same.
> 
>   [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb
>   [1]: https://lore.kernel.org/all/20241104171959.2938862-1-memxor@gmail.com
>   [2]: https://lore.kernel.org/bpf/20241206161053.809580-1-memxor@gmail.com
>   [3]: https://github.com/eddyz87/llvm-project/tree/nullness-for-tracepoint-params
> 
> Reported-by: Juri Lelli <juri.lelli@redhat.com> # original bug
> Reported-by: Manu Bretelle <chantra@meta.com> # bugs in masking fix
> Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
> Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> Co-developed-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Tbh, I think we should have fixed the bug in what is currently in the
tree and avoid revert. Anyways, the code looks good to me.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -6597,6 +6693,39 @@ 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;
> +
> +		/* BTF lookups cannot fail, return false on error */
> +		t = btf_type_by_id(btf, prog->aux->attach_btf_id);
> +		if (!t)
> +			return false;
> +		tname = btf_name_by_offset(btf, t->name_off);
> +		if (!tname)
> +			return false;
> +		/* Checked by bpf_check_attach_target */
> +		tname += sizeof("bpf_trace_") - 1;

Nit: bpf_check_attach_target uses "btf_trace_" prefix.

> +		for (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;
> +			if (raw_tp_null_args[i].mask & (0x1 << (arg * 4)))
> +				info->reg_type |= PTR_MAYBE_NULL;
> +			/* Is the current arg IS_ERR? */
> +			if (raw_tp_null_args[i].mask & (0x2 << (arg * 4)))
> +				ptr_err_raw_tp = true;
> +			break;
> +		}
> +		/* If we don't know NULL-ness specification and the tracepoint
> +		 * is coming from a loadable module, be conservative and mark
> +		 * argument as PTR_MAYBE_NULL.
> +		 */
> +		if (i == ARRAY_SIZE(raw_tp_null_args) && btf_is_module(btf))
> +			info->reg_type |= PTR_MAYBE_NULL;
> +	}
> +
>  	if (tgt_prog) {
>  		enum bpf_prog_type tgt_type;
>  
> @@ -6641,6 +6770,13 @@ 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.
> +	 */
> +	if (ptr_err_raw_tp)
> +		info->reg_type = SCALAR_VALUE;

Nit: the log line above would be a bit confusing if 'ptr_err_raw_tp' would be true.
     maybe add an additional line here, saying that verifier overrides BTF type?

>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(btf_ctx_access);

[...]