Re: [PATCH] libbpf: Fix integer overflow issue

[Andrii Nakryiko]

On Mon, Oct 7, 2024 at 12:13 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2024-10-08 at 00:46 +0800, I Hsin Cheng wrote:
> > Fix integer overflow issue discovered by coverity scan, where
> > "bpf_program_fd()" might return a value less than zero. Assignment of
> > "prog_fd" to "kern_data" will result in integer overflow in that case.
> >
> > Do a pre-check after the program fd is returned, if it's negative we
> > should ignore this program and move on, or maybe add some error handling
> > mechanism here.
> >
> > Signed-off-by: I Hsin Cheng <richard120310@xxxxxxxxx>
> > ---
> >  tools/lib/bpf/libbpf.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index a3be6f8fac09..95fb5e48e79e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8458,6 +8458,9 @@ static void bpf_map_prepare_vdata(const struct bpf_map *map)
> >                       continue;
> >
> >               prog_fd = bpf_program__fd(prog);
> > +             if (prog_fd < 0)
> > +                     continue;
> > +
>
> The 'progs' variable comes from 'st_ops->progs' array.
> Elements of this array are set in two places:
> a. bpf_object__collect_st_ops_relos() called from
>    bpf_object__collect_relos() called from
>    bpf_object_open().
>    This handles relocations pointing to programs in global struct ops
>    maps definitions, e.g.:
>
>     SEC(".struct_ops.link")
>     struct bpf_testmod_ops testmod_1 = {
>            .test_1 = (void *)test_1,     // <--- this one
>            ...
>     };
>
> b. bpf_map__init_kern_struct_ops() called from
>    bpf_object__init_kern_struct_ops_maps() called from
>    bpf_object_load().
>    This copies values set from "shadow types", e.g.:
>
>     skel->struct_ops.testmod_1->test_1 = skel->some_other_prog
>
> The bpf_map_prepare_vdata() itself is called from
> bpf_object_prepare_struct_ops() called from
> bpf_object_load().
>
> The call to bpf_object_prepare_struct_ops() is preceded by a call to
> bpf_object_adjust_struct_ops_autoload() (c), which in turn is preceded
> by both (a) and (b). Meaning that autoload decisions are final at the
> time of bpf_map_prepare_vdata() call. The (c) enables autoload for
> programs referenced from any struct ops map.
>
> Hence, I think that situation when 'prog_fd < 0' should not be
> possible here => we need an error log before skipping prog_fd
> (or aborting?).
>

Not sure what Eduard is suggesting here, tbh. But I think if this
actually can happen that we have a non-loaded BPF program in one of
those struct_ops slots, then let's add a test demonstrating that.

Worst case of what can happen right now is the kernel rejecting
struct_ops loading due to -22 as a program FD.

pw-bot: cr

> (Also, please double-check what Song Liu suggests about
>  obj->gen_loader, I am not familiar with that part).
>
> >               kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
> >               *(unsigned long *)kern_data = prog_fd;
> >       }
>
>