Re: [PATCH bpf-next v3 2/5] bpf: fix stream capacity leak in staging path

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

Pre-existing issues:
- [High] Unbounded memory growth caused by pushing zero-length strings, which completely bypasses the BPF stream capacity limits.
- [Medium] bpf_stream_vprintk() fails to cap the return value of bstr_printf(), causing valid truncated logs to be entirely dropped (returning -ENOMEM) and improperly injecting NUL bytes when lengths match the buffer size.
--

--- Patch [2]: [PATCH bpf-next v3 2/5] bpf: fix stream capacity leak in staging path ---
commit c8e97cb1d20310041c929271dc5ec88dac514fc8
Author: Jianlin Shi <[email protected]>
bpf: fix stream capacity leak in staging path

This patch fixes a capacity leak in the staging path of BPF streams by
accounting for the string length only after a successful push. It also
updates the string formatting to use vscnprintf(), correctly capping the
staged length to the truncated payload instead of the theoretical format
length.

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index 0b157ec4e38eb..ef5a7b06ffcb1 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c
> @@ -316,17 +316,19 @@ int bpf_stream_stage_printk(struct bpf_stream_stage *ss, const char *fmt, ...)
>  {
>  	struct bpf_bprintf_buffers *buf;
>  	va_list args;
> -	int ret;
> +	int len, ret;
>  
>  	if (bpf_try_get_buffers(&buf))
>  		return -EBUSY;
>  
>  	va_start(args, fmt);
> -	ret = vsnprintf(buf->buf, ARRAY_SIZE(buf->buf), fmt, args);
> +	/* Cap to the written length, excluding the trailing NUL. */
> +	len = vscnprintf(buf->buf, ARRAY_SIZE(buf->buf), fmt, args);
>  	va_end(args);

[Severity: Medium]
This isn't a bug introduced by this patch, but since you are fixing the
length truncation here, I noticed a similar issue in bpf_stream_vprintk()
with bstr_printf():

kernel/bpf/stream.c:bpf_stream_vprintk() {
	...
	ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt__str, data.bin_args);
	/* Exclude NULL byte during push. */
	ret = bpf_stream_push_str(stream, data.buf, ret);
	...
}

Does bstr_printf() return the theoretical untruncated string length, similar
to vsnprintf()? If the string is truncated, couldn't this attempt to push
out-of-bounds capacity or inject NUL bytes into the stream since the return
value is not capped?

> -	ss->len += ret;
>  	/* Exclude NULL byte during push. */
> -	ret = __bpf_stream_push_str(&ss->log, buf->buf, ret);
> +	ret = __bpf_stream_push_str(&ss->log, buf->buf, len);
> +	if (!ret)
> +		ss->len += len;
>  	bpf_put_buffers();
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential unbounded memory
growth when pushing zero-length strings.

Looking at bpf_stream_consume_capacity() and __bpf_stream_push_str():

kernel/bpf/stream.c:bpf_stream_consume_capacity() {
	...
	if (atomic_add_return(len, &stream->capacity) >= BPF_STREAM_MAX_CAPACITY) {
	...
}

kernel/bpf/stream.c:__bpf_stream_push_str() {
	...
	elem = bpf_stream_elem_alloc(len);
	if (!elem)
		return -ENOMEM;
	...
}

kernel/bpf/stream.c:bpf_stream_elem_alloc() {
	...
	alloc_size = offsetof(struct bpf_stream_elem, str[len]);
	elem = kmalloc_nolock(alloc_size, __GFP_ZERO, -1);
	...
}

If a BPF program repeatedly calls bpf_stream_stage_printk() or
bpf_stream_vprintk() with an empty string format (len = 0),
bpf_stream_consume_capacity() will add 0 to the capacity tracker, bypassing
the BPF_STREAM_MAX_CAPACITY limit entirely.

Meanwhile, __bpf_stream_push_str() still allocates structural memory for the
log element via kmalloc_nolock() even when length is 0.

Could this allow a BPF program to bypass the stream capacity limits and
continuously deplete kernel memory by flooding the log with zero-length
strings?

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