Re: [PATCH bpf 2/4] bpf, sockmap: Fix wrong rsge offset in bpf_msg_push_data()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] `bpf_msg_push_data` incorrectly reads the length of an uninitialized ring buffer slot when appending data to a full `sk_msg`, leading to ring buffer destruction and an infinite loop/crash during message free.
- [High] bpf_msg_push_data allocates uninitialized memory causing a kernel info leak.
--

commit d70a24eef21929f8e8a694f6c0b71f98f2d76089
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
> @@ -2828,8 +2828,6 @@ 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;

[Severity: Critical]
This is a pre-existing issue, but does this incorrectly read the length of an
uninitialized ring buffer slot when appending data to a full sk_msg?

When bpf_msg_push_data() is called to append data exactly at the end of a
full sk_msg (where space == 0), the scatterlist search loop finishes with
i == msg->sg.end. It then seems to read copy = msg->sg.data[i].length,
accessing an uninitialized length from the unused slot at msg->sg.end.

If this garbage copy value is > 0, the function allocates an oversized page
and sets the new scatterlist element's length to len + copy, but
msg->sg.size is only incremented by len. Could this cause msg->sg.size to
underflow during __sk_msg_free(), leading to an infinite loop and double
free?

If the garbage copy value is 0, the code later executes
sk_msg_iter_next(msg, end). Doesn't this increment msg->sg.end and wrap it
to msg->sg.start, destroying the ring buffer state and leading to
memory leaks or corruption?

>  
>  	if (unlikely(copy + len < copy))
>  		return -EINVAL;
>  
>  	page = alloc_pages(__GFP_NOWARN | GFP_ATOMIC | __GFP_COMP,
>  			   get_order(copy + len));

[Severity: High]
This isn't a bug introduced by this patch, but could this allocation be missing
the __GFP_ZERO flag?

When bpf_msg_push_data() inserts new space into a socket message, it
copies existing data to the front and back of the new page, but appears to
leave the newly inserted len bytes uninitialized.

In the non-copy path, the entire page is added to the scatterlist
uninitialized. Could this uninitialized kernel memory be exposed to the
sk_msg scatterlist and transmitted to the network or read by userspace via
tcp_bpf_push(), causing a kernel information leak?

It appears a later patch in the series (6cfebe959c2ffe911ff763388e942ea998f90682)
claims to fix this in its commit message, but it mistakenly patches
bpf_msg_pull_data() instead of bpf_msg_push_data(), leaving this issue
unfixed.

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

[ ... ]

> @@ -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);

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