Re: [PATCH] libbpf: Fix integer overflow issue

[Eduard Zingerman]

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

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