Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-08-21 at 12:10 -0700, Andrii Nakryiko wrote:
> On Wed, Aug 21, 2024 at 10:46 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote:
> > 
> > [...]
> > 
> > > > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> > > > Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> > > > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > ---
> > > >  tools/lib/bpf/relo_core.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > > > index 63a4d5ad12d1..a04724831ebc 100644
> > > > --- a/tools/lib/bpf/relo_core.c
> > > > +++ b/tools/lib/bpf/relo_core.c
> > > > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > > > 
> > > >         local_id = relo->type_id;
> > > >         local_type = btf_type_by_id(local_btf, local_id);
> > > > +       if (!local_type) {
> > > 
> > > This is a meaningless check at least for libbpf's implementation of
> > > btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
> > > tag clearly states the differences.
> > 
> > That is not true on kernel side.
> > bpf_core_calc_relo_insn() is called from bpf_core_apply():
> > 
> > int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> >                    int relo_idx, void *insn)
> > {
> >         bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
> >         ...
> >         if (need_cands) {
> >                 ...
> >                 // code below would report an error if relo->type_id is bogus
> >                 cc = bpf_core_find_cands(ctx, relo->type_id);
> >                 if (IS_ERR(cc)) {
> >                         bpf_log(ctx->log, "target candidate search failed for %d\n",
> >                                 relo->type_id);
> >                         err = PTR_ERR(cc);
> >                         goto out;
> >                 }
> >                 ...
> >         }
> > 
> >         err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
> >                                       &targ_res);
> >         ...
> > }
> > 
> > If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn().
> > Which is exactly the case with repro submitted by Liu.
> > There is also a simplified repro here:
> > https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17
> > But I can't submit it as a test yet.
> > 
> > > 
> > > So you'd need to validate local_id directly against number of types in
> > > local_btf.
> > 
> > How is this better than a null check?
> > 
> 
> because id check will be useful for both kernel and libbpf sides?..

This would require a special case for BPF_CORE_TYPE_ID_LOCAL in
bpf_core_calc_relo_insn(). If you don't like this null check I'll
modify bpf_core_apply() instead to always check relo->type_id and
report and error.

This would also be in the line with what bpf_core_resolve_relo()
does on libbpf side.

[...]