Re: [PATCH bpf-next v4 1/6] bpf, sockmap: reject overflowing copy + len in bpf_msg_push_data()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Appending to the end of a full or nearly full message scatterlist reads an out-of-bounds or unrelated `length`, causing a heap buffer overflow or massive information leak.
- [High] Allocating a new page without `__GFP_ZERO` in `bpf_msg_push_data()` leaks uninitialized kernel heap memory.
--

commit d474d4a6051d7bd721dc6d3aa5d5f0dff7f6ec9d
Author: Weiming Shi <bestswngs@gmail.com>

bpf, sockmap: reject overflowing copy + len in bpf_msg_push_data()

When the scatterlist ring is full or nearly full, bpf_msg_push_data()
enters a copy fallback path and computes copy + len for the page
allocation size. Since len comes from BPF with arg3_type = ARG_ANYTHING
and both are u32, a crafted len can wrap the sum to a small value,
causing an undersized allocation followed by an out-of-bounds memcpy.

Link: https://lore.kernel.org/all/20260424155913.A19FDC19425@smtp.kernel.org

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 57b00c6cc8cc1..4b159045881dd 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2829,6 +2829,9 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  	if (!space || (space == 1 && start != offset))
>  		copy = msg->sg.data[i].length;

[Severity: Critical]
This is a pre-existing issue, but does bpf_msg_push_data() read an
out-of-bounds or unrelated length when appending to the end of a full or
nearly full message scatterlist?

If appending exactly at the end of the message (start == offset + l), the loop
terminates with i == msg->sg.end. If there is exactly one available slot
(space == 1), the condition space == 1 && start != offset evaluates to true,
causing this line to execute.

Since i == msg->sg.end, this reads the length of an unused, empty slot (stale
data). This stale value dictates the allocation size and is added to the new
scatterlist element's length (msg->sg.data[new].length = len + copy). This
causes the scatterlist element to advertise a length much larger than the
actually initialized data, triggering massive out-of-bounds reads during
transmission and leaking thousands of bytes of kernel memory.

Additionally, if the scatterlist ring is entirely full (space == 0),
msg->sg.end equals msg->sg.start. Thus, copy incorrectly reads the length of
the first element in the ring, while the subsequent code consolidates the last
element. This results in allocating a page sized for the first element but
memcpy-ing the last element's data into it. If the last element is larger than
the first, this causes a deterministic heap buffer overflow.

>  
> +	if (unlikely(copy + len < copy))
> +		return -EINVAL;
> +
>  	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
>  			   get_order(copy + len));

[Severity: High]
This isn't a bug introduced by this patch, but does alloc_pages() here in
bpf_msg_push_data() leak uninitialized kernel heap memory?

This allocates a new page but intentionally skips initializing the newly
inserted len bytes region, copying only the front and back portions of the old
scatterlist element. Because the allocation does not include the __GFP_ZERO
flag, the inserted space contains whatever data previously occupied that kernel
heap page.

If the BPF program does not subsequently overwrite this region before the
message is transmitted, the uninitialized kernel memory is sent over the
network or exposed to user space.

>  	if (unlikely(!page))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615021959.140010-1-jiayuan.chen@linux.dev?part=1