netdev - Re: [PATCH] bpf: test_maps: Avoid possible out of bound access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 5 Feb 2019 11:23:58 -0200
From:   Breno Leitao <leitao@...ian.org>
To:     Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org
Cc:     ast@...nel.org
Subject: Re: [PATCH] bpf: test_maps: Avoid possible out of bound access

Hi Daniel,

On 2/4/19 6:57 PM, Daniel Borkmann wrote:
> There are couple more test_*() functions that need to be converted if we do
> the change to unsigned:
> 
> tools/testing/selftests/bpf/test_maps.c:48:static void test_hashmap(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:138:static void test_hashmap_sizes(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:158:static void test_hashmap_percpu(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:285:static void test_hashmap_walk(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:356:static void test_arraymap(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:411:static void test_arraymap_percpu(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:507:static void test_devmap(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:522:static void test_queuemap(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:580:static void test_stackmap(int task, void *data)
> tools/testing/selftests/bpf/test_maps.c:645:static void test_sockmap(int tasks, void *data)

That is a good idea. I haven't change them because the 'task' argument was
not being used at all, hence no GCC warning also. But due to the
__run_parallel() function parameter, I agree that we need to change them all.

Also, we need to change the test_update_delete(unsigned int fn, void *data)
function, which is also called through __run_parallel():

	run_parallel(TASKS, test_update_delete, data);

>>  {
>>  	struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_msg, *bpf_map_break;
>>  	int map_fd_msg = 0, map_fd_rx = 0, map_fd_tx = 0, map_fd_break;
>> @@ -1261,7 +1261,7 @@ static void test_map_large(void)
>>  	printf("Fork %d tasks to '" #FN "'\n", N); \
>>  	__run_parallel(N, FN, DATA)
>>  
>> -static void __run_parallel(int tasks, void (*fn)(int task, void *data),
>> +static void __run_parallel(unsigned int tasks, void (*fn)(int task, void *data),
> 
> This would also need conversion to unsigned for the func arg above so that
> we don't type mismatch.

Ack!

I am sending a V2 soon. Than you!