Re: [PATCH bpf 3/3] samples/bpf: syscall_tp_user: Fix array out-of-bound access

Jinghao Jia
2023-08-28T23:17:30Z

[-- Attachment #1.1: Type: text/plain, Size: 3293 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]