Re: [PATCH bpf-next 2/2] selftests/bpf: Check that bpftool reports a map it can't dump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: uninitialized output buffer in test_bpftool_map_dump()
--

commit 18cf283c6b4e8fb98efa7b6286ae3efd8620915d
Author: Maxim Skokov <skokovmaksimevg@gmail.com>

selftests/bpf: Check that bpftool reports a map it can't dump

This commit adds a new test to verify that bpftool map dump correctly
reports an error when trying to dump a socket storage map, rather than
exiting silently. It creates a BPF_MAP_TYPE_SK_STORAGE map, attaches
storage to a socket, pins it, and runs bpftool to check the output.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
> new file mode 100644
> index 0000000000000..56283e3f1e602
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
[ ... ]
> +void test_bpftool_map_dump(void)
> +{
> +	int map_fd, sk_fd = -1, value = 42, ret;
> +	char output[1024];
[ ... ]
> +	/* Socket storage can not be iterated, so the dump must fail loudly
> +	 * rather than print an empty map that holds an element.
> +	 */
> +	ret = get_bpftool_command_output("-j map dump pinned " PIN_PATH,
> +					 output, sizeof(output));
> +	ASSERT_NEQ(ret, 0, "json dump fails");
> +	ASSERT_HAS_SUBSTR(output, "\"error\":", "json dump reports an error");

[Severity: Medium]
Could this result in an uninitialized memory read if get_bpftool_command_output()
fails early?

If bpftool path detection fails or popen() fails inside run_command(),
it returns 1 without writing anything to the buffer:

tools/testing/selftests/bpf/bpftool_helpers.c:run_command() {
	if (bpftool_path[0] == 0 && detect_bpftool_path(bpftool_path, sizeof(bpftool_path)))
		return 1;
	...
	f = popen(command, "r");
	if (!f)
		return 1;
	...
}

In this scenario, get_bpftool_command_output() will return 1, which satisfies
ASSERT_NEQ(ret, 0). The test will then pass the uninitialized output array to
ASSERT_HAS_SUBSTR(), which calls strstr() and may cause undefined behavior or
crashes.

Should output be zero-initialized, or should the test verify that output was
actually populated before checking its contents?

> +
> +	ret = get_bpftool_command_output("map dump pinned " PIN_PATH " 2>&1",
> +					 output, sizeof(output));
> +	ASSERT_NEQ(ret, 0, "plain dump fails");
> +	ASSERT_HAS_SUBSTR(output, "does not support iteration",
> +			  "plain dump explains why");
> +
> +	unlink(PIN_PATH);
> +out:
> +	if (sk_fd >= 0)
> +		close(sk_fd);
> +	close(map_fd);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924160732.485650-1-skokovmaksimevg@gmail.com?part=2