On Tue, Dec 10, 2024 at 6:02 PM 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. > > 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 > reflets the current best known list according to Jiri's analysis in [3]. > > [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > [1]: https://lore.kernel.org/all/20241104171959.2938862-1-memxor@xxxxxxxxx > [2]: https://lore.kernel.org/bpf/20241206161053.809580-1-memxor@xxxxxxxxx > [3]: https://lore.kernel.org/bpf/Z1d-qbCdtJqg6Er4@krava > > Reported-by: Juri Lelli <juri.lelli@xxxxxxxxxx> # original bug > Reported-by: Manu Bretelle <chantra@xxxxxxxx> # 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@xxxxxxxxxx> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > --- > kernel/bpf/btf.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 129 insertions(+) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index ed3219da7181..cb72cbf04d12 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6439,6 +6439,96 @@ int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto, > return off; > } > > +struct bpf_raw_tp_null_args { > + const char *func; > + u64 mask; > +}; > + > +#define RAW_TP_NULL_ARGS(str, arg) { .func = "btf_trace_" #str, .mask = (arg) } > +/* Use 1-based indexing for argno */ > +#define NULL_ARG(argno) (1 << (argno)) > + > +struct bpf_raw_tp_null_args raw_tp_null_args[] = { > + /* sched */ > + RAW_TP_NULL_ARGS(sched_pi_setprio, NULL_ARG(2)), > + /* ... from sched_numa_pair_template event class */ > + RAW_TP_NULL_ARGS(sched_stick_numa, NULL_ARG(3)), Let's avoid LOUD macros. "btf_trace_" prefix can also be dropped to save space. How about the following encoding: {"sched_pi_setprio", 0x10}, {"sched_stick_numa", 0x100}, > + RAW_TP_NULL_ARGS(cachefiles_lookup, NULL_ARG(1)), {"cachefiles_lookup", 0x1}, There is no need for arg0, since the count starts from arg1. > + 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; +1 to Jiri's point. Let's use 0x2 to encode the scalar ?
