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

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

[Kunwu Chan]

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

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 1/4] selftests/bpf: Add some null pointer checks

[Kunwu Chan]

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 1/4] selftests/bpf: Add some null pointer checks

[Markus Elfring]

…
> This patch will add the malloc failure checking
…

* Please use a corresponding imperative wording for the change description.

* Would you like to add the tag “Fixes” accordingly?


…
> +++ 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;
> +	}
…

How do you think about to reuse “errno” in such error cases?

Regards,
Markus

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

[Kunwu Chan]

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 2/4] selftests/bpf/sockopt: Add a null pointer check for the run_test

[Markus Elfring]

…
> This patch will add the malloc failure checking
…

* Please use a corresponding imperative wording for the change description.

* Would you like to add the tag “Fixes” accordingly?


Regards,
Markus

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

[Kunwu Chan]

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 3/4] selftests/bpf: Add a null pointer check for the load_btf_spec

[Markus Elfring]

…
> Add the malloc failure checking to avoid possible null
> dereference.
…

How do you think about the following wording variant?

   Add a return value check so that a null pointer dereference will be avoided
   after a memory allocation failure.


Would you like to add the tag “Fixes” accordingly?


…
> +++ 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;
…

How do you think about to reuse the variable “errno” in such an error case?

Regards,
Markus

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

[Kunwu Chan]

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/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..302b25408a53 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 (CHECK(!query, "malloc()", "error:%s\n", strerror(errno)))
+		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 4/4] selftests/bpf: Add a null pointer check for the serial_test_tp_attach_query

[Markus Elfring]

…
> Add the malloc failure checking to avoid possible null
> dereference.
…

How do you think about to use the following wording variant?

   Add a return value check so that a null pointer dereference will be avoided
   after a memory allocation failure.


Would you like to add the tag “Fixes” accordingly?

Regards,
Markus

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

[Daniel Borkmann]

On 4/24/24 4:04 AM, Kunwu Chan wrote:
> 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/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..302b25408a53 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 (CHECK(!query, "malloc()", "error:%s\n", strerror(errno)))

Series looks reasonable, small nit on CHECK() : Lets use ASSERT*() macros given they are
preferred over the latter :

if (!ASSERT_OK_PTR(buf, "malloc"))

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

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

[Daniel Borkmann]

On 5/3/24 5:47 PM, Daniel Borkmann wrote:
> On 4/24/24 4:04 AM, Kunwu Chan wrote:
>> 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/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..302b25408a53 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 (CHECK(!query, "malloc()", "error:%s\n", strerror(errno)))
> 
> Series looks reasonable, small nit on CHECK() : Lets use ASSERT*() macros given they are
> preferred over the latter :
> 
> if (!ASSERT_OK_PTR(buf, "malloc"))

( Also as a side-note: Fixes tag on all these patches is not needed given this will just
   end up spamming stable tree. If you indeed end up with NULL then the tests will just
   segfault & fail. )

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

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

[Kunwu Chan]

Thanks all for your reply.

On 2024/5/3 23:47, Daniel Borkmann wrote:
> On 4/24/24 4:04 AM, Kunwu Chan wrote:
>> 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/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..302b25408a53 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 (CHECK(!query, "malloc()", "error:%s\n", strerror(errno)))
>
> Series looks reasonable, small nit on CHECK() : Lets use ASSERT*() 
> macros given they are
> preferred over the latter :
>
> if (!ASSERT_OK_PTR(buf, "malloc"))

Thanks, I'll update it in v2:

1: Use ASSERT_OK_PTR instead of CHECK

2: Add a suggested-by tag for you

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