linux-kernel - Re: [PATCH] libbpf: Fix integer overflow issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3be8b6307e7576e5a654f42414a1f0f45a754901.camel@gmail.com>
Date: Mon, 07 Oct 2024 12:13:43 -0700
From: Eduard Zingerman <eddyz87@...il.com>
To: I Hsin Cheng <richard120310@...il.com>, martin.lau@...ux.dev
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 song@...nel.org,  yonghong.song@...ux.dev, john.fastabend@...il.com,
 kpsingh@...nel.org,  sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org,
 bpf@...r.kernel.org,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libbpf: Fix integer overflow issue

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@...il.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;
>  	}