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