Re: [PATCH net 1/1] bpf: sockmap: fix tail fragment offset in bpf_msg_push_data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] bpf_msg_push_data allocates memory for new message data without the __GFP_ZERO flag, leaving the newly inserted bytes uninitialized and exposing kernel heap memory.
- [High] When appending data to a full or nearly full sk_msg, bpf_msg_push_data incorrectly reads the length from the inactive msg->sg.end index, bypassing the coalescing logic and causing a ring buffer wrap-around, out-of-bounds access, or information leak.
- [High] Calling bpf_msg_push_data on an empty sk_msg causes the scatterlist iteration loop to traverse inactive ring buffer slots, leading to ring buffer corruption.
- [High] Shifting scatterlist elements in bpf_msg_push_data and bpf_msg_pop_data does not properly shift the msg->sg.copy bitmap, breaking the Copy-On-Write mechanism.
- [High] The len parameter in bpf_msg_push_data is not bounds-checked, allowing a maliciously large len to cause an integer overflow, resulting in a small allocation followed by a massive heap out-of-bounds write.
--

commit fefea84ac268cf25e699730bfbde310ae44b83f8
Author: Yuqi Xu <xuyq21@lenovo.com>

bpf: sockmap: fix tail fragment offset in bpf_msg_push_data

When bpf_msg_push_data() inserts data in the middle of a scatterlist
entry, it splits the original entry into a left fragment and a right
fragment. The right fragment offset is advanced with the message-global
insertion point, which over-advances the offset. This commit advances
the offset by the fragment-local delta instead.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..80439767e0eea 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2869,7 +2869,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,

[Severity: High]
This isn't a bug introduced by this patch, but when appending data to a full
or nearly full sk_msg, does bpf_msg_push_data incorrectly read the length
from the inactive msg->sg.end index?

If a BPF program appends data to the end of a message, the search loop
terminates with i == msg->sg.end. If the ring buffer is full (space == 0),
copy = msg->sg.data[i].length reads from the inactive slot.

If the stale length is 0, the if (copy) block is skipped. The function then
calls sk_msg_iter_next(msg, end) on a full ring buffer, wrapping msg->sg.end
to equal msg->sg.start, logically emptying the message and leaking all
original pages.

If the stale length is non-zero, it sizes the new allocation incorrectly. If
the previous element's length exceeds the size of the allocation order for
copy + len, the subsequent memcpy will overflow the allocated heap buffer.

[Severity: High]
This is a pre-existing issue, but can calling bpf_msg_push_data on an empty
sk_msg cause the scatterlist iteration loop to traverse inactive ring buffer
slots?

If the message is completely empty (e.g., drained via bpf_msg_pop_data),
msg->sg.start == msg->sg.end.

The do { ... } while (i != msg->sg.end) loop executes at least once because
of the do-while construct. It traverses uninitialized or stale elements,
accumulating stale length values into offset.

If the loop condition start < offset + l fails to break, it iterates through
the entire ring buffer, leaving offset heavily corrupted. This leads to
front = start - offset becoming a large negative number, causing a massive
out-of-bounds memcpy that will reliably panic the kernel.

[Severity: High]
This isn't a bug introduced by this patch, but could shifting scatterlist
elements in bpf_msg_push_data and bpf_msg_pop_data fail to properly shift
the msg->sg.copy bitmap, breaking the Copy-On-Write mechanism?

msg->sg.copy is a bitmap used to track which scatterlist elements are shared
(e.g., from TCP zerocopy) and require a private copy before modification.

When elements are shifted in the ring buffer during bpf_msg_push_data
(shifted right) or bpf_msg_pop_data (shifted left), their corresponding
msg->sg.copy bits are not shifted.

This desynchronization clears the copy protection for elements that moved.
A BPF program calling bpf_msg_pull_data on these elements will be granted
direct write access instead of triggering a copy, allowing it to corrupt
shared memory or page-cache data.

[Severity: Critical]
This is a pre-existing issue, but is bpf_msg_push_data allocating memory
for new message data without the __GFP_ZERO flag?

bpf_msg_push_data allocates new pages via alloc_pages(__GFP_NOWARN |
GFP_ATOMIC | __GFP_COMP, ...). The kernel inserts len bytes into the page
but never initializes this specific region.

A BPF program can request this memory expansion without writing any data to
it. If the sk_msg is subsequently transmitted over a network socket, the
uninitialized kernel heap memory will be sent across the network, leading
to a significant information leak.

[Severity: High]
This isn't a bug introduced by this patch, but is the len parameter in
bpf_msg_push_data missing a bounds check?

The len parameter is a u32 value passed from the BPF program, with no
verifier bounds check (ARG_ANYTHING). An excessively large len
(e.g., 0xFFFFFFFF) causes the 32-bit addition copy + len to overflow to a
small value.

alloc_pages successfully allocates a small buffer. During the fallback copy
logic, the pointer arithmetic to = raw + front + len expands the 32-bit len
to a 64-bit unsigned offset, advancing the destination pointer by ~4GB.

The subsequent memcpy(to, from, back) performs an out-of-bounds write on
the heap, allowing a verified BPF program to corrupt kernel memory and cause
a kernel panic.

>  
>  		psge->length = start - offset;
>  		rsge.length -= psge->length;
> -		rsge.offset += start;
> +		rsge.offset += start - offset;
>  
>  		sk_msg_iter_var_next(i);
>  		sg_unmark_end(psge);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/8b129d10566aa3eb43f61a8f9757bcf51707d324.1779636774.git.xuyq21@lenovo.com?part=1