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@gmail.com>
---
 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;
+
 		kern_data = st_ops->kern_vdata + st_ops->kern_func_off[i];
 		*(unsigned long *)kern_data = prog_fd;
 	}
-- 
2.43.0

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@gmail.com>
> ---
>  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?).

(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;
>  	}

On Mon, Oct 7, 2024 at 12:13 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com>
> > ---
> >  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;
> >       }
>
>

On Mon, 2024-10-07 at 20:42 -0700, Andrii Nakryiko wrote:

[...]

> 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.

Given the call chain listed in a previous email I think that such
situation is not possible (modulo obj->gen_loader, which I know
nothing about).

Thus I suggest to add a pr_warn() and return -EINVAL or something like
that here.

> 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

[...]

On Tue, Oct 8, 2024 at 2:49 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2024-10-07 at 20:42 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > 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.
>
> Given the call chain listed in a previous email I think that such
> situation is not possible (modulo obj->gen_loader, which I know
> nothing about).
>
> Thus I suggest to add a pr_warn() and return -EINVAL or something like
> that here.
>

That's what confused me :) If it's impossible, there is no need to
handle it, we know the FD has to be there. So I'd just not change
anything.

> > 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
>
> [...]
>

On Tue, 2024-10-08 at 10:21 -0700, Andrii Nakryiko wrote:
> On Tue, Oct 8, 2024 at 2:49 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Mon, 2024-10-07 at 20:42 -0700, Andrii Nakryiko wrote:
> > 
> > [...]
> > 
> > > 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.
> > 
> > Given the call chain listed in a previous email I think that such
> > situation is not possible (modulo obj->gen_loader, which I know
> > nothing about).
> > 
> > Thus I suggest to add a pr_warn() and return -EINVAL or something like
> > that here.
> > 
> 
> That's what confused me :) If it's impossible, there is no need to
> handle it, we know the FD has to be there. So I'd just not change
> anything.

Granted I have a memory of a fruit fly, but it took me like half an
hour to figure out if it is possible or not, and I wrote a part of
that code. At the very least a comment is needed.
Also, adding an explicit cast should silence the tool warning.

> 
> > > 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
> > 
> > [...]
> > 

On Mon, Oct 7, 2024 at 9:46 AM I Hsin Cheng <richard120310@gmail.com> 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.

Has this been a real issue other than coverity scan? If so, we will need
a Fix tag.

Also, some logistics. Please be clear which tree this patch targets,
and tag the patches with "[PATCH bpf]" or "[PATCH bpf-next]".

> 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@gmail.com>
> ---
>  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;
> +

AFAICT, this only happens with non-NULL obj->gen_loader. So we can
achieve the same with something like:

diff --git i/tools/lib/bpf/libbpf.c w/tools/lib/bpf/libbpf.c
index 712b95e8891b..6756199a942f 100644
--- i/tools/lib/bpf/libbpf.c
+++ w/tools/lib/bpf/libbpf.c
@@ -8502,6 +8502,8 @@ static int bpf_object_prepare_struct_ops(struct
bpf_object *obj)
        struct bpf_map *map;
        int i;

+       if (obj->gen_loader)
+               return 0;
        for (i = 0; i < obj->nr_maps; i++) {
                map = &obj->maps[i];


Thanks,
Song

On Tue, Oct 08, 2024 at 12:46:48AM +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.

We already had a mechanism there - the one you'd just disabled.
Namely, storing an unsigned long value with MSB set at given
offset.