On Fri, 6 Dec 2024 at 19:15, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2024-12-06 at 09:59 -0800, Alexei Starovoitov wrote:
> > On Fri, Dec 6, 2024 at 8:11 AM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> > >
> > > An implication of this fix, which follows from the way the raw_tp fixes
> > > were implemented, is that all PTR_MAYBE_NULL trusted PTR_TO_BTF_ID are
> > > engulfed by these checks, and PROBE_MEM will apply to all of them, incl.
> > > those coming from helpers with KF_ACQUIRE returning maybe null trusted
> > > pointers. This NULL tagging after this commit will be sticky. Compared
> > > to a solution which only specially tagged raw_tp args with a different
> > > special maybe null tag (like PTR_SOFT_NULL), it's a consequence of
> > > overloading PTR_MAYBE_NULL with this meaning.
> > >
> > > Fixes: cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> > > Reported-by: Manu Bretelle <chantra@xxxxxxxx>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > ---
> > > kernel/bpf/verifier.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 82f40d63ad7b..556fb609d4a4 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -15365,6 +15365,12 @@ static void mark_ptr_or_null_reg(struct bpf_verifier_env *env,
> > > return;
> > >
> > > if (is_null) {
> > > + /* We never mark a raw_tp trusted pointer as scalar, to
> > > + * preserve backwards compatibility, instead just leave
> > > + * it as is.
> > > + */
> > > + if (mask_raw_tp_reg_cond(env, reg))
> > > + return;
> >
> > The blast radius is getting too big.
> > Patch 1 is ok, but here we're doubling down on
> > the hack in commit
> > cb4158ce8ec8 ("bpf: Mark raw_tp arguments with PTR_MAYBE_NULL")
> >
> > I think we need to revert the raw_tp masking hack and
> > go with denylist the way Jiri proposed:
> > https://lore.kernel.org/bpf/ZrIj9jkXqpKXRuS7@krava/
> >
> > denylist is certainly less safer and it's a whack-a-mole
> > comparing to allowlist, but it's much much shorter
> > according to Jiri's analysis:
> > https://lore.kernel.org/bpf/Zr3q8ihbe8cUdpfp@krava/
> >
> > Eduard had an idea how to auto generate such allow/denylist
> > during the build.
> > That could be a follow up.
>
> If the sole goal is to avoid dead code elimination for tracepoint
> parameter null check, there might be another hack. Not sure if it was
> discussed:
> - don't add PTR_MAYBE_NULL (but maybe add a new tag, PTR_SOFT_NULL
> from Kumar's original RFC);
> - in is_branch_taken() don't predict anything when tracepoint
> parameters are compared;
> - in mark_ptr_or_null_regs() don't propagate null for pointers to
> tracepoint parameters (as in this patch).
That was pretty much the first attempt with a soft null tag, it was a
special tag indicating provenance of the argument, so it only applied
to raw_tp args. I was trying to warn when being passed into helpers
and kfuncs too, but that needs a more complete fix anyway (with
PTR_TO_BTF_ID...) so we decided to address it later.
But at this point I think we'll keep digging ourselves into a deeper
hole the more we try to address it this way.
I think the various issues, breakage, and corner cases are evidence
this is probably not a good approach to take.
Longer term we have to do explicit annotations and know when it is
NULL and not NULL, so it is probably better to bite the bullet now and
do it explicitly.
Once we denylist or handle the ones Jiri found, we can keep discussing
how to keep the list up to date.
>
> Seems pretty confined but can't catch nullable tracepoint parameters
> being passed to kfuncs.
>
