Re: [PATCH bpf-next v4 3/5] bpf: return partial progress from bpf_stream_read on fault

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The length parameter passed from userspace is cast from an unsigned 32-bit integer to a signed integer, allowing negative values that bypass bounds checks and result in massive out-of-bounds reads via `copy_to_user()`.
--

--- Patch [3]: [PATCH bpf-next v4 3/5] bpf: return partial progress from bpf_stream_read on fault ---
commit 4510393da76186ad876fa17b3e2eb0055506983a
Author: Jianlin Shi <shijianlin11@foxmail.com>

bpf: return partial progress from bpf_stream_read on fault

bpf_stream_read() pops and frees stream elements after a successful
copy_to_user(). If a later copy_to_user() fails, it currently restores
only the current element's consumed_len and returns -EFAULT, hiding
bytes already delivered to userspace and making the consumed data
unrecoverable on retry.

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index a36fc93389677..09471a6052682 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c
> @@ -180,13 +181,14 @@ static int bpf_stream_read(struct bpf_stream *stream, void __user *buf, int len)
>  
>  		cons_len = elem->consumed_len;
>  		cont = bpf_stream_consume_elem(elem, &rem_len) == false;
> -
> -		ret = copy_to_user(buf + pos, elem->str + cons_len,
> -				   elem->consumed_len - cons_len);
> -		/* Restore in case of error. */
> -		if (ret) {
> -			ret = -EFAULT;
> -			elem->consumed_len = cons_len;
> +		chunk = elem->consumed_len - cons_len;
> +
> +		n = copy_to_user(buf + pos, elem->str + cons_len, chunk);

[Severity: High]
This isn't a bug introduced by this patch, but could this chunk size end up
as a large negative number, causing a massive out-of-bounds read?

If a user issues BPF_PROG_STREAM_READ_BY_FD with a stream_buf_len greater
than INT_MAX (e.g. 0x80000000), the length is read as an unsigned 32-bit
integer from userspace:

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);
}

This is then implicitly cast to a signed int here in bpf_stream_read(). If
len becomes negative, rem_len also starts negative.

When bpf_stream_consume_elem() is called, it calculates used = min(rem, *len).
Because *len (which is rem_len) is negative, used evaluates to a negative
value.

This makes chunk a negative value, which is implicitly cast to an enormous
unsigned long when passed to copy_to_user(). This could lead to a guaranteed
kernel panic or denial of service when hitting unmapped kernel pages.

Should len be bounds checked or treated as unsigned int to prevent this?

> +		if (n) {
> +			/* Keep any successfully copied bytes; -EFAULT only if none. */
> +			elem->consumed_len -= n;
> +			rem_len += n;
> +			ret = (len == rem_len) ? -EFAULT : 0;
>  			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787492521.git.shijianlin11@foxmail.com?part=3