Re: [PATCH v3 bpf-next 3/5] selftests/bpf: Fix missing allocation null checks in test_progs.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Early return from `dispatch_thread_send_subtests` leaves `subtest_states` partially uninitialized, leading to a crash during cleanup.
- [High] Missing NULL check for `strdup` in `dispatch_thread_send_subtests` leads to a potential segmentation fault.
- [High] Updating `subtest_num` before allocating `subtest_states` leads to a NULL pointer dereference during cleanup if an error occurs.
- [Medium] Failing `realloc` in `test__start_subtest_with_desc` overwrites the original pointer, leaking the array.
--

--- Patch [3]: [PATCH v3 bpf-next 3/5] selftests/bpf: Fix missing allocation null checks in test_progs.c ---
commit d01ffcd5c8b16e028e42c669b9b1234582456211
Author: Feng Yang <yangfeng@kylinos.cn>

selftests/bpf: Fix missing allocation null checks in test_progs.c

Add null checks after memory allocations to prevent potential segmentation faults.

Fixes: 79b453501310 ("tools/bpf: add a test for bpf_get_stack with tracepoint prog")
Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")

> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 312743c4337fd..301c6e11ceafa 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c

[ ... ]

> @@ -1514,6 +1517,10 @@ 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));

[Severity: High]
This isn't a bug introduced by this patch, but because malloc() does not
zero-initialize memory, could the early return below cause a segmentation fault
during test teardown?

If read_prog_test_msg() returns an error, dispatch_thread_send_subtests()
returns 1:

tools/testing/selftests/bpf/test_progs.c:dispatch_thread_send_subtests() {
    ...
	for (int i = 0; i < subtest_num; i++) {
		subtest_state = &state->subtest_states[i];
		memset(subtest_state, 0, sizeof(*subtest_state));

		if (read_prog_test_msg(sock_fd, &msg, MSG_SUBTEST_DONE))
			return 1;
    ...
}

Any elements after index 'i' are left containing uninitialized memory. Later,
free_test_states() iterates through the entire array based on subtest_num and
attempts to free the pointers in each element:

tools/testing/selftests/bpf/test_progs.c:free_test_states() {
    ...
	for (j = 0; j < test_state->subtest_num; j++)
		free_subtest_state(&test_state->subtest_states[j]);
    ...
}

tools/testing/selftests/bpf/test_progs.c:free_subtest_state() {
    ...
	free(state->log_buf);
    ...
}

Would it be safer to use calloc() instead of malloc() here to guarantee the
entire array is zero-initialized?

> +	if (!state->subtest_states) {
> +		state->subtest_num = 0;
> +		return -ENOMEM;
> +	}
>  
>  	for (int i = 0; i < subtest_num; i++) {
>  		subtest_state = &state->subtest_states[i];

[Severity: High]
This is a pre-existing issue, but does the strdup() call later in this
function need a NULL check?

tools/testing/selftests/bpf/test_progs.c:dispatch_thread_send_subtests() {
    ...
		if (read_prog_test_msg(sock_fd, &msg, MSG_SUBTEST_DONE))
			return 1;

		subtest_state->name = strdup(msg.subtest_done.name);
    ...
}

If memory allocation fails, subtest_state->name becomes NULL. When
dump_test_log() processes the results, it passes this to jsonw_string_field()
which eventually reaches jsonw_puts(). Since jsonw_puts() unconditionally loops
over the string characters, passing it a NULL pointer leads to a segmentation
fault.

[Severity: High]
This is also a pre-existing issue, but does updating state->subtest_num before
allocating state->subtest_states lead to a NULL pointer dereference?

In dispatch_thread(), subtest_num is set before the array is allocated:

tools/testing/selftests/bpf/test_progs.c:dispatch_thread() {
    ...
			state->subtest_num = msg.test_done.subtest_num;

			/* collect all logs */
			if (msg.test_done.have_log) {
				if (dispatch_thread_read_log(sock_fd,
							     &state->log_buf,
							     &state->log_cnt))
					goto error;
			}
    ...
}

If dispatch_thread_read_log() fails, the thread exits on error. During
cleanup, free_test_states() iterates up to state->subtest_num:

tools/testing/selftests/bpf/test_progs.c:free_test_states() {
    ...
	for (j = 0; j < test_state->subtest_num; j++)
		free_subtest_state(&test_state->subtest_states[j]);
    ...
}

Because subtest_states is still NULL, free_subtest_state() receives a NULL
pointer (or near-NULL for j > 0) and attempts to free state->log_buf from it,
resulting in a crash.

[Severity: Medium]
This is another pre-existing issue, but does this realloc() pattern leak the
subtest_states array on failure?

tools/testing/selftests/bpf/test_progs.c:test__start_subtest_with_desc() {
    ...
	state->subtest_states =
		realloc(state->subtest_states,
			state->subtest_num * sub_state_size);
    ...
}

If realloc() fails, it returns NULL and overwrites the original pointer, losing
the only reference to the previously allocated memory block.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723085100.482147-1-yangfeng59949@163.com?part=3