| Message ID | 20230818164643.97782-4-jinghao@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | samples/bpf: syscall_tp_user: Refactor and fix array index out-of-bounds bug | expand |
On 8/18/23 6:46 PM, Jinghao Jia wrote: > Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint > to syscall_tp sample") added two more eBPF programs to support the > openat2() syscall. However, it did not increase the size of the array > that holds the corresponding bpf_links. This leads to an out-of-bound > access on that array in the bpf_object__for_each_program loop and could > corrupt other variables on the stack. On our testing QEMU, it corrupts > the map1_fds array and causes the sample to fail: > > # ./syscall_tp > prog #0: map ids 4 5 > verify map:4 val: 5 > map_lookup failed: Bad file descriptor > > Dynamically allocate the array based on the number of programs reported > by libbpf to prevent similar inconsistencies in the future > > Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample") > Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com> > --- > samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c > index 18c94c7e8a40..8855d2c1290d 100644 > --- a/samples/bpf/syscall_tp_user.c > +++ b/samples/bpf/syscall_tp_user.c > @@ -48,7 +48,7 @@ static void verify_map(int map_id) > static int test(char *filename, int nr_tests) > { > int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0; > - struct bpf_link *links[nr_tests * 4]; > + struct bpf_link **links = NULL; > struct bpf_object *objs[nr_tests]; > struct bpf_program *prog; > > @@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests) > goto cleanup; > } > > + /* One-time initialization */ > + if (!links) { > + int nr_progs = 0; > + > + bpf_object__for_each_program(prog, objs[i]) > + nr_progs += 1; > + > + links = calloc(nr_progs * nr_tests, > + sizeof(struct bpf_link *)); NULL check is missing > + } > + > /* load BPF program */ > if (bpf_object__load(objs[i])) { > fprintf(stderr, "loading BPF object file failed\n"); > @@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests) > } > > cleanup: > - for (j--; j >= 0; j--) > - bpf_link__destroy(links[j]); > + if (links) { > + for (j--; j >= 0; j--) > + bpf_link__destroy(links[j]); > + > + free(links); > + links = NULL; why is this explicit links = NULL needed? > + } > > for (i--; i >= 0; i--) > bpf_object__close(objs[i]); >
On 8/25/23 09:57, Daniel Borkmann wrote: > On 8/18/23 6:46 PM, Jinghao Jia wrote: >> Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint >> to syscall_tp sample") added two more eBPF programs to support the >> openat2() syscall. However, it did not increase the size of the array >> that holds the corresponding bpf_links. This leads to an out-of-bound >> access on that array in the bpf_object__for_each_program loop and could >> corrupt other variables on the stack. On our testing QEMU, it corrupts >> the map1_fds array and causes the sample to fail: >> >> # ./syscall_tp >> prog #0: map ids 4 5 >> verify map:4 val: 5 >> map_lookup failed: Bad file descriptor >> >> Dynamically allocate the array based on the number of programs reported >> by libbpf to prevent similar inconsistencies in the future >> >> Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample") >> Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com> >> --- >> samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++--- >> 1 file changed, 19 insertions(+), 3 deletions(-) >> >> diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c >> index 18c94c7e8a40..8855d2c1290d 100644 >> --- a/samples/bpf/syscall_tp_user.c >> +++ b/samples/bpf/syscall_tp_user.c >> @@ -48,7 +48,7 @@ static void verify_map(int map_id) >> static int test(char *filename, int nr_tests) >> { >> int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0; >> - struct bpf_link *links[nr_tests * 4]; >> + struct bpf_link **links = NULL; >> struct bpf_object *objs[nr_tests]; >> struct bpf_program *prog; >> @@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests) >> goto cleanup; >> } >> + /* One-time initialization */ >> + if (!links) { >> + int nr_progs = 0; >> + >> + bpf_object__for_each_program(prog, objs[i]) >> + nr_progs += 1; >> + >> + links = calloc(nr_progs * nr_tests, >> + sizeof(struct bpf_link *)); > > NULL check is missing Oh, apparently I missed that, will fix in v2. > >> + } >> + >> /* load BPF program */ >> if (bpf_object__load(objs[i])) { >> fprintf(stderr, "loading BPF object file failed\n"); >> @@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests) >> } >> cleanup: >> - for (j--; j >= 0; j--) >> - bpf_link__destroy(links[j]); >> + if (links) { >> + for (j--; j >= 0; j--) >> + bpf_link__destroy(links[j]); >> + >> + free(links); >> + links = NULL; > > why is this explicit links = NULL needed? Yeah I agree this is redundant, since links is not used later. > >> + } >> for (i--; i >= 0; i--) >> bpf_object__close(objs[i]); >> > > > I wonder if there are further comments before I roll out a v2? --Jinghao
diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c index 18c94c7e8a40..8855d2c1290d 100644 --- a/samples/bpf/syscall_tp_user.c +++ b/samples/bpf/syscall_tp_user.c @@ -48,7 +48,7 @@ static void verify_map(int map_id) static int test(char *filename, int nr_tests) { int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0; - struct bpf_link *links[nr_tests * 4]; + struct bpf_link **links = NULL; struct bpf_object *objs[nr_tests]; struct bpf_program *prog; @@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests) goto cleanup; } + /* One-time initialization */ + if (!links) { + int nr_progs = 0; + + bpf_object__for_each_program(prog, objs[i]) + nr_progs += 1; + + links = calloc(nr_progs * nr_tests, + sizeof(struct bpf_link *)); + } + /* load BPF program */ if (bpf_object__load(objs[i])) { fprintf(stderr, "loading BPF object file failed\n"); @@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests) } cleanup: - for (j--; j >= 0; j--) - bpf_link__destroy(links[j]); + if (links) { + for (j--; j >= 0; j--) + bpf_link__destroy(links[j]); + + free(links); + links = NULL; + } for (i--; i >= 0; i--) bpf_object__close(objs[i]);
Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample") added two more eBPF programs to support the openat2() syscall. However, it did not increase the size of the array that holds the corresponding bpf_links. This leads to an out-of-bound access on that array in the bpf_object__for_each_program loop and could corrupt other variables on the stack. On our testing QEMU, it corrupts the map1_fds array and causes the sample to fail: # ./syscall_tp prog #0: map ids 4 5 verify map:4 val: 5 map_lookup failed: Bad file descriptor Dynamically allocate the array based on the number of programs reported by libbpf to prevent similar inconsistencies in the future Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample") Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com> --- samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)