[PATCH bpf-next v2 0/4] Add some 'malloc' failure checks

[PATCH bpf-next v2 0/4] Add some 'malloc' failure checks

[kunwu.chan]

From: Kunwu Chan <chentao@kylinos.cn>

The "malloc" call may not be successful.Add the malloc
failure checking to avoid possible null dereference.

Changes since v1 [1]:
        - As Daniel Borkmann suggested, change patch 4/4 only
        - Other 3 patches no changes.

[1] https://lore.kernel.org/all/20240424020444.2375773-1-chentao@kylinos.cn/

Kunwu Chan (4):
  selftests/bpf: Add some null pointer checks
  selftests/bpf/sockopt: Add a null pointer check for the run_test
  selftests/bpf: Add a null pointer check for the load_btf_spec
  selftests/bpf: Add a null pointer check for the
    serial_test_tp_attach_query

 tools/testing/selftests/bpf/prog_tests/sockopt.c         | 6 ++++++
 tools/testing/selftests/bpf/prog_tests/tp_attach_query.c | 3 +++
 tools/testing/selftests/bpf/test_progs.c                 | 7 +++++++
 tools/testing/selftests/bpf/test_verifier.c              | 2 ++
 4 files changed, 18 insertions(+)

-- 
2.40.1

[PATCH bpf-next v2 1/4] selftests/bpf: Add some null pointer checks

[kunwu.chan]

From: Kunwu Chan <chentao@kylinos.cn>

There is a 'malloc' call, which can be unsuccessful.
This patch will add the malloc failure checking
to avoid possible null dereference.

Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 89ff704e9dad..ecc3ddeceeeb 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -582,6 +582,11 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
 
 	val_buf1 = malloc(stack_trace_len);
 	val_buf2 = malloc(stack_trace_len);
+	if (!val_buf1 || !val_buf2) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	cur_key_p = NULL;
 	next_key_p = &key;
 	while (bpf_map_get_next_key(smap_fd, cur_key_p, next_key_p) == 0) {
@@ -1197,6 +1202,8 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
 	int subtest_num = state->subtest_num;
 
 	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));
+	if (!state->subtest_states)
+		return -ENOMEM;
 
 	for (int i = 0; i < subtest_num; i++) {
 		subtest_state = &state->subtest_states[i];
-- 
2.40.1

Re: [PATCH bpf-next v2 1/4] selftests/bpf: Add some null pointer checks

[Muhammad Usama Anjum]

On 5/10/24 2:58 PM, kunwu.chan@linux.dev wrote:
> From: Kunwu Chan <chentao@kylinos.cn>
> 
> There is a 'malloc' call, which can be unsuccessful.
> This patch will add the malloc failure checking
> to avoid possible null dereference.
> 
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 89ff704e9dad..ecc3ddeceeeb 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -582,6 +582,11 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
>  
>  	val_buf1 = malloc(stack_trace_len);
>  	val_buf2 = malloc(stack_trace_len);
> +	if (!val_buf1 || !val_buf2) {
> +		err = -ENOMEM;
Return from here instead of going to out where free(val_buf*) is being called.

> +		goto out;
> +	}
> +
>  	cur_key_p = NULL;
>  	next_key_p = &key;
>  	while (bpf_map_get_next_key(smap_fd, cur_key_p, next_key_p) == 0) {
> @@ -1197,6 +1202,8 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
>  	int subtest_num = state->subtest_num;
>  
>  	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));
> +	if (!state->subtest_states)
> +		return -ENOMEM;
>  
>  	for (int i = 0; i < subtest_num; i++) {
>  		subtest_state = &state->subtest_states[i];

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH bpf-next v2 1/4] selftests/bpf: Add some null pointer checks

[Kunwu Chan]

On 2024/5/10 19:20, Muhammad Usama Anjum wrote:
> On 5/10/24 2:58 PM, kunwu.chan@linux.dev wrote:
>> From: Kunwu Chan <chentao@kylinos.cn>
>>
>> There is a 'malloc' call, which can be unsuccessful.
>> This patch will add the malloc failure checking
>> to avoid possible null dereference.
>>
>> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
>> ---
>>   tools/testing/selftests/bpf/test_progs.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
>> index 89ff704e9dad..ecc3ddeceeeb 100644
>> --- a/tools/testing/selftests/bpf/test_progs.c
>> +++ b/tools/testing/selftests/bpf/test_progs.c
>> @@ -582,6 +582,11 @@ int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
>>   
>>   	val_buf1 = malloc(stack_trace_len);
>>   	val_buf2 = malloc(stack_trace_len);
>> +	if (!val_buf1 || !val_buf2) {
>> +		err = -ENOMEM;
> Return from here instead of going to out where free(val_buf*) is being called.
I think it's no harm.  And Unify the processing at the end to achieve 
uniform format.
>> +		goto out;
>> +	}
>> +
>>   	cur_key_p = NULL;
>>   	next_key_p = &key;
>>   	while (bpf_map_get_next_key(smap_fd, cur_key_p, next_key_p) == 0) {
>> @@ -1197,6 +1202,8 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
>>   	int subtest_num = state->subtest_num;
>>   
>>   	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));
>> +	if (!state->subtest_states)
>> +		return -ENOMEM;
>>   
>>   	for (int i = 0; i < subtest_num; i++) {
>>   		subtest_state = &state->subtest_states[i];

-- 
Thanks,
   Kunwu.Chan

Re: [PATCH bpf-next v2 1/4] selftests/bpf: Add some null pointer checks

[Markus Elfring]

> There is a 'malloc' call, which can be unsuccessful.

                  two calls?


> This patch will add the malloc failure checking
…

Please use imperative wordings for improved change descriptions also in your patches.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n94

Regards,
Markus

[PATCH bpf-next v2 2/4] selftests/bpf/sockopt: Add a null pointer check for the run_test

[kunwu.chan]

From: Kunwu Chan <chentao@kylinos.cn>

There is a 'malloc' call, which can be unsuccessful.
Add the malloc failure checking to avoid possible null
dereference and give more information
about test fail reasons.

Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
---
 tools/testing/selftests/bpf/prog_tests/sockopt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
index 5a4491d4edfe..bde63e1f74dc 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
@@ -1100,6 +1100,12 @@ static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
 		}
 
 		optval = malloc(test->get_optlen);
+		if (!optval) {
+			log_err("Failed to allocate memory");
+			ret = -1;
+			goto close_sock_fd;
+		}
+
 		memset(optval, 0, test->get_optlen);
 		socklen_t optlen = test->get_optlen;
 		socklen_t expected_get_optlen = test->get_optlen_ret ?:
-- 
2.40.1

Re: [PATCH bpf-next v2 2/4] selftests/bpf/sockopt: Add a null pointer check for the run_test

[Muhammad Usama Anjum]

On 5/10/24 2:58 PM, kunwu.chan@linux.dev wrote:
> From: Kunwu Chan <chentao@kylinos.cn>
> 
> There is a 'malloc' call, which can be unsuccessful.
> Add the malloc failure checking to avoid possible null
> dereference and give more information
> about test fail reasons.
> 
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
LGTM
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> ---
>  tools/testing/selftests/bpf/prog_tests/sockopt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt.c b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> index 5a4491d4edfe..bde63e1f74dc 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt.c
> @@ -1100,6 +1100,12 @@ static int run_test(int cgroup_fd, struct sockopt_test *test, bool use_io_uring)
>  		}
>  
>  		optval = malloc(test->get_optlen);
> +		if (!optval) {
> +			log_err("Failed to allocate memory");
> +			ret = -1;
> +			goto close_sock_fd;
> +		}
> +
>  		memset(optval, 0, test->get_optlen);
>  		socklen_t optlen = test->get_optlen;
>  		socklen_t expected_get_optlen = test->get_optlen_ret ?:

-- 
BR,
Muhammad Usama Anjum

[PATCH bpf-next v2 3/4] selftests/bpf: Add a null pointer check for the load_btf_spec

[kunwu.chan]

From: Kunwu Chan <chentao@kylinos.cn>

There is a 'malloc' call, which can be unsuccessful.
Add the malloc failure checking to avoid possible null
dereference.

Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
---
 tools/testing/selftests/bpf/test_verifier.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index df04bda1c927..9c80b2943418 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -762,6 +762,8 @@ static int load_btf_spec(__u32 *types, int types_len,
 	);
 
 	raw_btf = malloc(sizeof(hdr) + types_len + strings_len);
+	if (!raw_btf)
+		return -ENOMEM;
 
 	ptr = raw_btf;
 	memcpy(ptr, &hdr, sizeof(hdr));
-- 
2.40.1

Re: [PATCH bpf-next v2 3/4] selftests/bpf: Add a null pointer check for the load_btf_spec

[Muhammad Usama Anjum]

On 5/10/24 2:58 PM, kunwu.chan@linux.dev wrote:
> From: Kunwu Chan <chentao@kylinos.cn>
> 
> There is a 'malloc' call, which can be unsuccessful.
> Add the malloc failure checking to avoid possible null
> dereference.
> 
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
LGTM
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> ---
>  tools/testing/selftests/bpf/test_verifier.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index df04bda1c927..9c80b2943418 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -762,6 +762,8 @@ static int load_btf_spec(__u32 *types, int types_len,
>  	);
>  
>  	raw_btf = malloc(sizeof(hdr) + types_len + strings_len);
> +	if (!raw_btf)
> +		return -ENOMEM;
>  
>  	ptr = raw_btf;
>  	memcpy(ptr, &hdr, sizeof(hdr));

-- 
BR,
Muhammad Usama Anjum

[PATCH bpf-next v2 4/4] selftests/bpf: Add a null pointer check for the serial_test_tp_attach_query

[kunwu.chan]

From: Kunwu Chan <chentao@kylinos.cn>

There is a 'malloc' call, which can be unsuccessful.
Add the malloc failure checking to avoid possible null
dereference.

Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
---
Changes in v2:
	- Use ASSERT* instead of CHECK
	- Add suggested-by tag
---
 tools/testing/selftests/bpf/prog_tests/tp_attach_query.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
index 655d69f0ff0b..a5ebfc172ad8 100644
--- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
+++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
@@ -39,6 +39,9 @@ void serial_test_tp_attach_query(void)
 	attr.wakeup_events = 1;
 
 	query = malloc(sizeof(*query) + sizeof(__u32) * num_progs);
+	if (!ASSERT_OK_PTR(query, "malloc"))
+		return;
+
 	for (i = 0; i < num_progs; i++) {
 		err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj[i],
 				    &prog_fd[i]);
-- 
2.40.1

Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add a null pointer check for the serial_test_tp_attach_query

[Muhammad Usama Anjum]

On 5/10/24 2:58 PM, kunwu.chan@linux.dev wrote:
> From: Kunwu Chan <chentao@kylinos.cn>
> 
> There is a 'malloc' call, which can be unsuccessful.
> Add the malloc failure checking to avoid possible null
> dereference.
> 
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> Changes in v2:
> 	- Use ASSERT* instead of CHECK
> 	- Add suggested-by tag
> ---
>  tools/testing/selftests/bpf/prog_tests/tp_attach_query.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
> index 655d69f0ff0b..a5ebfc172ad8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
> @@ -39,6 +39,9 @@ void serial_test_tp_attach_query(void)
>  	attr.wakeup_events = 1;
>  
>  	query = malloc(sizeof(*query) + sizeof(__u32) * num_progs);
> +	if (!ASSERT_OK_PTR(query, "malloc"))
> +		return;
> +
LGTM
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

>  	for (i = 0; i < num_progs; i++) {
>  		err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj[i],
>  				    &prog_fd[i]);

-- 
BR,
Muhammad Usama Anjum