Re: [PATCH bpf-next v4 5/5] selftests/bpf: cover stream capacity and partial read edge cases

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

New issues:
- [Medium] The `test_stream_oversize` test fails to verify its stated claim that an oversized push does not permanently consume stream capacity.

Pre-existing issues:
- [Critical] Unvalidated `len` parameter in `prog_stream_read` can be cast to a negative integer, leading to a massive out-of-bounds read of kernel memory on 32-bit systems.
--

--- Patch [5]: [PATCH bpf-next v4 5/5] selftests/bpf: cover stream capacity and partial read edge cases ---
commit f20a230c79a9f103af40866a70610a281adb0a0f
Author: Jianlin Shi <[email protected]>

selftests/bpf: cover stream capacity and partial read edge cases

This patch adds test coverage for recent fixes to BPF stream capacity
and partial reads. It verifies that an oversized bpf_stream_printk()
does not leak capacity and that bpf_prog_stream_read() handles
unmapped pages correctly.

> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> index e4e9374309e26..9a21f055d80aa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stream.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c
[ ... ]
> @@ -58,6 +58,71 @@ void test_stream_syscall(void)
> +void test_stream_oversize(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, opts);
> +	struct stream *skel;
> +	int ret, prog_fd;
> +	char buf[8] = {};
> +
> +	skel = stream__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "stream__open_and_load"))
> +		return;
> +
> +	prog_fd = bpf_program__fd(skel->progs.stream_oversize);
> +	ret = bpf_prog_test_run_opts(prog_fd, &opts);
> +	ASSERT_OK(ret, "oversize run");
> +	ASSERT_OK(opts.retval, "oversize retval");
> +
> +	/* Oversized push must not permanently consume capacity on this prog. */
> +	ret = bpf_prog_stream_read(prog_fd, BPF_STREAM_STDOUT, buf, sizeof(buf), NULL);

[Severity: High]
This is a pre-existing issue, but can the length parameter passed to
bpf_prog_stream_read() cause an out-of-bounds read?

In kernel/bpf/syscall.c:prog_stream_read(), an unbounded u32 length
from user space is passed down:

kernel/bpf/syscall.c:prog_stream_read() {
    u32 len = attr->prog_stream_read.stream_buf_len;
    ...
    ret = bpf_prog_stream_read(prog, attr->prog_stream_read.stream_id, buf, len);
}

Since bpf_prog_stream_read() takes an int, large values (e.g. 0x80000000)
are cast to negative integers.

This negative length bypasses the loop exit conditions in
kernel/bpf/stream.c:bpf_stream_read():

kernel/bpf/stream.c:bpf_stream_read() {
    while (rem_len) {
        ...
        cont = bpf_stream_consume_elem(elem, &rem_len) == false;
        chunk = elem->consumed_len - cons_len;
        n = copy_to_user(buf + pos, elem->str + cons_len, chunk);
    }
}

Could this result in a negative chunk size being passed to copy_to_user(),
which is then cast to a massive unsigned value on 32-bit systems?

> +	ASSERT_EQ(ret, 3, "bytes after oversize");
> +	ASSERT_OK(memcmp(buf, "foo", 3), "payload after oversize");
> +
> +	stream__destroy(skel);
> +}
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> index 8e8e1339dc743..02fe07a00cebd 100644
> --- a/tools/testing/selftests/bpf/progs/stream.c
> +++ b/tools/testing/selftests/bpf/progs/stream.c
[ ... ]
> @@ -119,6 +124,19 @@ int stream_syscall(void *ctx)
> +SEC("syscall")
> +__success __retval(0)
> +int stream_oversize(void *ctx)
> +{
> +	int ret;
> +
> +	ret = bpf_stream_printk(BPF_STDOUT, _BIG_STR);
> +	if (ret != -E2BIG)
> +		return ret;
> +
> +	return bpf_stream_printk(BPF_STDOUT, "foo");

[Severity: Medium]
Does this code actually verify that capacity is not leaked?

Since a single oversized push of 1024 bytes leaves most of the capacity
unused, won't this subsequent write of "foo" succeed even if a leak
occurred?

Would it be better to loop and exhaust the capacity limit to properly
detect a leak?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5