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@xxxxxxxxxxxxx>
>> ---
>> 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
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
