| Message ID | 20241007164648.20926-1-richard120310@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Fix integer overflow issue | expand |
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.
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, 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 > > > > [...] > >
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; }
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(+)