Re: NULL pointer deref when running BPF monitor program (6.11.0-rc1)

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Aug 06, 2024 at 11:44:52AM -0700, Alexei Starovoitov wrote:
> On Tue, Aug 6, 2024 at 6:24 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > > Jiri,
> > >
> > > the verifier removes the check because it assumes that pointers
> > > passed by the kernel into tracepoint are valid and trusted.
> > > In this case:
> > >         trace_sched_pi_setprio(p, pi_task);
> > >
> > > pi_task can be NULL.
> > >
> > > We cannot make all tracepoint pointers to be PTR_TRUSTED | PTR_MAYBE_NULL
> > > by default, since it will break a bunch of progs.
> > > Instead we can annotate this tracepoint arg as __nullable and
> > > teach the verifier to recognize such special arguments of tracepoints.
> >
> > ok, so you mean to be able to mark it in event header like:
> >
> >   TRACE_EVENT(sched_pi_setprio,
> >         TP_PROTO(struct task_struct *tsk, struct task_struct *pi_task __nullable),
> >
> > I guess we could make pahole to emit DECL_TAG for that argument,
> > but I'm not sure how to propagate that __nullable info to pahole
> >
> > while wondering about that, I tried the direct fix below ;-)
> 
> We don't need to rush such a hack below.
> No need to add decl_tag and change pahole either.
> The arg name is already vmlinux BTF:
> [51371] FUNC_PROTO '(anon)' ret_type_id=0 vlen=3
>         '__data' type_id=61
>         'tsk' type_id=77
>         'pi_task' type_id=77
> [51372] FUNC '__bpf_trace_sched_pi_setprio' type_id=51371 linkage=static
> 
> just need to rename "pi_task" to "pi_task__nullable"
> and teach the verifier.

the problem is that btf_trace_<xxx> is typedef 

  typedef void (*btf_trace_##call)(void *__data, proto);

and dwarf does not store argument names for subroutine type entry,
so it's not in BTF's TYPEDEF either

it's the btf_trace_##call typedef ID that verifier has to work with,
I wonder we could somehow associate that ID with __bpf_trace_##call
subroutine entry which has the argument names

we could store __bpf_trace_##call's BTF_ID in __bpf_raw_tp_map record,
but we'd need to do the lookup based on the tracepoint name when loading
the program .. ATM we do the lookup __bpf_raw_tp_map record only when
doing attach, so we would need to move it to program load time

or we could 'fix' the argument names in pahole, but that'd probably
mean extra setup and hash lookup, so also not great

jirka


> 
> 
> > jirka
> >
> >
> > ---
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 95426d5b634e..1a20bbdead64 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6377,6 +6377,25 @@ int btf_ctx_arg_offset(const struct btf *btf, const struct btf_type *func_proto,
> >         return off;
> >  }
> >
> > +static bool is_tracing_prog_raw_tp(const struct bpf_prog *prog, const char *name)
> > +{
> > +       struct btf *btf = prog->aux->attach_btf;
> > +       const struct btf_type *t;
> > +       const char *tname;
> > +
> > +       if (prog->expected_attach_type != BPF_TRACE_RAW_TP)
> > +               return false;
> > +
> > +       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;
> > +       return !strcmp(tname, name);
> > +}
> > +
> >  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >                     const struct bpf_prog *prog,
> >                     struct bpf_insn_access_aux *info)
> > @@ -6544,6 +6563,10 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >                 }
> >         }
> >
> > +       /* Second argument of sched_pi_setprio tracepoint can be null */
> > +       if (is_tracing_prog_raw_tp(prog, "btf_trace_sched_pi_setprio") && arg == 1)
> > +               info->reg_type |= PTR_MAYBE_NULL;
> > +
> >         info->btf = btf;
> >         info->btf_id = t->type;
> >         t = btf_type_by_id(btf, t->type);