Re: [PATCH bpf v2 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 scatterlist length from an unused ring buffer slot when appending to the end of a message, leading to severe memory corruption, buffer overflows, and ring corruption.
--

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

bpf, sockmap: Fix wrong rsge offset in bpf_msg_push_data()

This commit fixes an issue in bpf_msg_push_data() where splitting a scatterlist
element incorrectly advanced the tail's page offset by the absolute message
byte offset (start) instead of the byte position within the element
(start - offset). This caused rsge.offset to overshoot, leading to
out-of-bounds accesses.

> 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,

[Severity: Critical]
This is a pre-existing issue, but does bpf_msg_push_data() read a stale
length from an unused ring buffer slot when appending to the end of a message?

When appending data to the end of the sk_msg ring (where
start == msg->sg.size), the loop iterating over the scatterlist evaluates
all valid elements and terminates with i == msg->sg.end:

net/core/filter.c:bpf_msg_push_data() {
    ...
	do {
		offset += l;
		l = sk_msg_elem(msg, i)->length;

		if (start < offset + l)
			break;
		sk_msg_iter_var_next(i);
	} while (i != msg->sg.end);
    ...
}

After this loop, the code evaluates the space and reads the copy length:

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

Because start != offset evaluates to true when appending, this reads
msg->sg.data[msg->sg.end].length. This slot is outside the valid ring
bounds and contains a stale, garbage length from prior operations.

This garbage copy value can lead to severe memory corruption and buffer
overflows. For example, if copy is non-zero, it allocates a buffer based
on copy + len. Later, it copies front bytes into the new allocation using
memcpy:

	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
			   get_order(copy + len));
    ...
		if (front)
			memcpy(raw, from, front);

Since front is the size of the actual last element, and the stale copy
value can be small, front can easily exceed the allocated page size,
causing memcpy to overflow the allocated page.

Additionally, if the stale copy value happens to be 0 on a completely full
ring, the code skips the allocation, executes sk_msg_iter_next(msg, end)
unconditionally, and wraps msg->sg.end to equal msg->sg.start, permanently
leaking all message pages.

Could this garbage read lead to memory corruption, buffer overflows, and
ring corruption?

>  
>  		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/20260611123538.156005-1-jiayuan.chen@linux.dev?part=2