Re: [PATCH bpf] bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

[sashiko-bot@kernel.org]

Sashiko AI review found 6 potential issue(s):
- [Critical] Unbounded `len` causes `copy + len` integer overflow, leading to an exploitable out-of-bounds kernel write.
- [Critical] Appending to a full `sk_msg` wraps the ring buffer index, corrupting the last element and causing an out-of-bounds read/write.
- [Critical] Scatterlist element shifting desynchronizes `msg->sg.copy` bitmap, allowing unauthorized direct writes to shared pages.
- [High] Incorrect scatterlist offset calculation when splitting elements causes out-of-bounds memory read.
- [High] Appending to an `sk_msg` with 1 free slot reads garbage length, inflating payload size and leaking uninitialized memory.
- [Medium] Unnecessary zeroing of compound pages in atomic context causes latency spikes.
--

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

bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data

The commit attempts to fix a kernel heap information leak in
bpf_msg_push_data() by adding __GFP_ZERO to the alloc_pages() call. This
zeroes out the allocated pages before they are added to the scatterlist,
preventing uninitialized memory from being sent 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 5fa9189eb772b2..3422cbf09c7da2 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2820,7 +2820,7 @@ 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;

This isn't a bug introduced by this patch, but when appending data (start ==
msg->sg.size) and exactly 1 free slot remains (space == 1), the search loop
exits with i == msg->sg.end. Does this fallback path read from an unused ring
buffer slot containing uninitialized garbage, since
msg->sg.data[msg->sg.end].length is used here?

Later, this garbage value is added to len to set the new element's length,
inflating it far beyond the copied data.

Also, when appending data to the exact end of a message with no free slots
(space == 0), the search loop terminates with i == msg->sg.end and
msg->sg.end wraps to msg->sg.start. Does this code mistakenly copy the
length of the first element in the ring?

Then when calculating back:

	if (i == msg->sg.end)
		sk_msg_iter_var_prev(i);
	psge = sk_msg_elem(msg, i);
	front = start - offset;
	back = psge->length - front;

If the first element is shorter than the last, couldn't back become negative
and pass a large unsigned size to memcpy(), leading to an out-of-bounds
write?

> -	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
> +	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP | __GFP_ZERO,
>  			   get_order(copy + len));

Does copy + len overflow here if a BPF program passes a very large len
(e.g., 0xFFFFF000)?

Since both are 32-bit integers, couldn't this wrap around to a small positive
value, causing get_order() to allocate a much smaller buffer than required,
and later lead to an out-of-bounds memcpy() when to = raw + front + len?

Additionally, if a BPF program passes a valid len that requires a high-order
compound page (e.g., 32KB or 64KB), will synchronously zeroing the entire
memory block under GFP_ATOMIC with preemption disabled introduce latency spikes
into the network fast-path?

Would it be better to allocate normally and use memset() to zero only the
exact gap of uninitialized len bytes?

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

This isn't a bug introduced by this patch, but later in the same function when
inserting data into the middle of a scatterlist element:

	if (start - offset) {
		if (i == msg->sg.end)
			sk_msg_iter_var_prev(i);
		psge = sk_msg_elem(msg, i);
		rsge = sk_msg_elem_cpy(msg, i);

		psge->length = start - offset;
		rsge.length -= psge->length;
		rsge.offset += start;

Is rsge.offset incorrectly advanced by the absolute logical byte offset
start?

Shouldn't rsge.offset instead be advanced by the relative number of bytes
consumed by the first half (which is start - offset)? Does this absolute
offset addition cause an out-of-bounds memory read when transmitting the
scatterlist element?

Lastly, further down in the function when shifting entries:

	while (i != msg->sg.end) {
		msg->sg.data[i] = sge;
		sge = nsge;
		sk_msg_iter_var_next(i);
		if (rsge.length) {
			nsge = nnsge;
			nnsge = sk_msg_elem_cpy(msg, i);
		} else {

When bpf_msg_push_data() shifts existing scatterlist entries to the right to
make room for new elements, it appears to miss shifting the corresponding bits
in the msg->sg.copy bitmap.

Does this desynchronize the copy-on-write tracking?

If a shared page is shifted to an index with a cleared bitmap bit, could a
subsequent bpf_msg_pull_data() directly overwrite memory belonging to other
sockets or the page cache?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424190310.1520555-2-bestswngs@gmail.com?part=1