Re: [PATCH bpf-next v4 7/8] selftests/bpf: Add tests to verify global percpu data

On 15/4/26 05:45, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
> - [Medium] The tests use the raw CPU ID to index into the densely packed `percpu_data` array, which will cause an out-of-bounds read and test failures on systems with sparse CPU masks.
> --
> 
> commit a7c2685b539ebc816cdb05decab1d7a8a4e1328e
> Author: Leon Hwang <[email protected]>
>     
> selftests/bpf: Add tests to verify global percpu data
>     
> This commit adds new selftests to verify the behavior of global percpu data
> by checking map initial values, struct pointers, and looking up elements from
> global percpu data maps.
> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data_init.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
> 
> [ ... ]
> 
>> +static void test_global_percpu_data_init(void)
>> +{
> 
> [ ... ]
> 
>> +	err = bpf_map__lookup_elem(map, &key, sizeof(key), percpu_data,
>> +				   elem_sz * num_cpus, 0);
>> +	if (!ASSERT_OK(err, "bpf_map__lookup_elem"))
>> +		goto out;
>> +
>> +	for (i = 0; i < num_online; i++) {
>> +		if (!online[i])
>> +			continue;
>> +
>> +		data = (void *)percpu_data + elem_sz * i;
> 
> Will this logic cause an out-of-bounds read on systems with sparse CPU masks?
> 
> When reading a per-CPU map using bpf_map__lookup_elem(), the kernel packs the
> per-CPU values densely into the output buffer by iterating over the possible
> CPU mask.
> 
> The test allocates num_cpus (the total count of possible CPUs) elements for
> the output buffer, but then iterates over the raw CPU IDs using i and uses i
> as an index into the densely packed array.
> 
> On systems with sparse CPU masks (for example, where CPUs 0 and 100 are
> possible/online), num_cpus will be 2. When the loop reaches i = 100, it
> appears it will attempt to access percpu_data + elem_sz * 100, reading past
> the bounds of the allocated memory.
> 
> Could we maintain a separate index counter that increments only for possible
> CPUs to match the kernel's dense packing behavior?
> 
> [ ... ]
> 
>> +static void test_global_percpu_data_lskel(void)
>> +{
> 
> [ ... ]
> 
>> +	err = bpf_map_lookup_elem(map_fd, &key, percpu_data);
>> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
>> +		goto out;
>> +
>> +	for (i = 0; i < num_online; i++) {
>> +		if (!online[i])
>> +			continue;
>> +
>> +		data = (void *)percpu_data + elem_sz * i;
> 
> Does this loop have the same bounds checking issue as mentioned above?
> 
> If i is a raw CPU ID on a system with a sparse CPU mask, this calculation
> could exceed the bounds of the percpu_data buffer.
> 

Yep, agree with the review.

Instead of lookup values across all possible CPUs, it should lookup
value on specified CPU using BPF_F_CPU, because the test only runs on
online CPUs.

Thanks,
Leon