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(+)
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];
…
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
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 ?:
…
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
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));
…
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
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]);
…
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
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]);
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]);
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]);
linux-kselftest-mirror@lists.linaro.org
-
Daniel Borkmann -
Kunwu Chan -
Kunwu Chan -
Markus Elfring