Re: [PATCH bpf-next v3 3/7] bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] bpf_msg_push_data() reads from an unused scatterlist element when appending data to a full ring buffer, leading to ring buffer collapse and memory leak.
--

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

bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

This patch ensures that pages allocated in bpf_msg_push_data() are zeroed
by adding the __GFP_ZERO flag to the alloc_pages() call. This prevents
stale heap content, including kernel pointers, from being transmitted to
userspace.

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

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3e555f276ba80..6e345ca65ca14 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2832,7 +2832,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  	if (unlikely(copy + len < copy))
>  		return -EINVAL;
>  
> -	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
> +	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP | __GFP_ZERO,
>  			   get_order(copy + len));

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
bpf_msg_push_data() in net/core/filter.c to read from an unused scatterlist
element when appending data to a full ring buffer?

When appending data to the end of the message (start == total_length), the
scatterlist iteration loop exits with i == msg->sg.end.

If the ring buffer is full (space == 0), the code attempts to coalesce the new
data with an existing fragment to free up a slot:

	if (!space || (space == 1 && start != offset))
		copy = msg->sg.data[i].length;

Since i == msg->sg.end, this points to the unused, zero-initialized separator
slot.

As a result, copy evaluates to 0, the coalescing path is skipped, and the new
fragment is unconditionally appended to the full ring buffer.

Does this unconditionally advance msg->sg.end and cause it to equal
msg->sg.start (since MAX_MSG_FRAGS + 1 wraps around)?

If so, sk_msg_elem_used() would return 0, making the message appear empty,
collapsing the entire ring buffer, and leaking all previously allocated
scatterlist pages.

>  	if (unlikely(!page))
>  		return -ENOMEM;

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