Re: [PATCH bpf-next v3 2/7] bpf, sockmap: Fix wrong rsge offset 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:
- [Critical] bpf_msg_push_data() reads a stale length from an unused scatterlist slot, leading to ring buffer corruption and heap buffer overflow.
--

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

    bpf, sockmap: Fix wrong rsge offset in bpf_msg_push_data()
    
    When bpf_msg_push_data() splits a scatterlist element into head and
    tail, the tail's page offset is advanced by `start` (absolute message
    byte offset) instead of `start - offset` (byte position within the
    element). This makes rsge.offset overshoot by `offset` bytes, pointing
    to the wrong location within the page or beyond its boundary. Consumers
    of the corrupted entry either silently read wrong data or trigger an
    out-of-bounds access.
    
    [ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3c8f1cedb217f..3e555f276ba80 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2872,7 +2872,7 @@ BPF_CALL_4(bpf_msg_push_data, struct sk_msg *, msg, u32, start,
>  
>  		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);

[Severity: Critical]
This is a pre-existing issue, but I have a question about how we handle
scatterlist elements when appending data in bpf_msg_push_data().

When appending data to the end of a message (where start == msg->sg.size),
the search loop exits with i == msg->sg.end. If the ring buffer is full or
near full (space <= 1), the fallback path initializes copy:

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

Could this read a stale length from an unused scatterlist slot since i points
to msg->sg.end?

If copy > 0, we then allocate a page using this stale copy value:

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

And later, fetch the actual last element and copy data into it:

	if (copy) {
		...
		if (i == msg->sg.end)
			sk_msg_iter_var_prev(i);
		psge = sk_msg_elem(msg, i);
		front = start - offset;
		...
		if (front)
			memcpy(raw, from, front);

If the page order determined by the stale copy value is smaller than what
psge->length requires, can this result in an out-of-bounds write and
corrupt adjacent kernel memory?

Additionally, if the stale length is 0, the allocation succeeds but the copy
path is bypassed. The code later does:

	/* Slot(s) to place newly allocated data */
	sk_msg_iter_next(msg, end);

If the ring was full (space == 0), does advancing msg->sg.end wrap it to
equal start, incorrectly signaling an empty ring buffer and leaking all
existing fragments?

Note: An uninitialized memory leak here was fixed by a subsequent patch
"bpf, sockmap: zero-initialize pages allocated in bpf_msg_push_data", but
these corruption and overflow paths appear to remain unaddressed.

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