Re: [PATCH bpf] bpf, sockmap: fix integer overflow in bpf_msg_pop_data() bounds check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbounded `len` parameter in `bpf_msg_push_data` causes a 32-bit integer overflow when calculating the page allocation size (`copy + len`), resulting in an out-of-bounds heap write.
- [High] Incorrect scatterlist element offset calculation when splitting an element in `bpf_msg_push_data` inflates the page offset, leading to out-of-bounds memory accesses.
--

commit 619f5b1d1aebdbd6457c3ac6e3a92bc45be7278c
Author: Sechang Lim <rhkrqnwk98@gmail.com>

bpf, sockmap: fix integer overflow in bpf_msg_pop_data() bounds check

start and len are u32, so start + len evaluates in 32-bit and wraps before
storing it in last. The bounds check can then be passed with an out-of-range
start/len.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..31467c0b4279e 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2974,7 +2974,7 @@ BPF_CALL_4(bpf_msg_pop_data, struct sk_msg *, msg, u32, start,
>  	   u32, len, u64, flags)
>  {
>  	u32 i = 0, l = 0, space, offset = 0;
> -	u64 last = start + len;
> +	u64 last = (u64)start + len;

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the integer
overflow in bpf_msg_pop_data(), could a similar overflow occur in
bpf_msg_push_data()?

Looking at net/core/filter.c:bpf_msg_push_data(), both copy and len are u32:

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

If a large len is provided, copy + len could wrap around to a small value,
resulting in a much smaller allocation than required.

Later in the same function:

			to = raw + front + len;

			memcpy(to, from, back);

Since len is zero-extended to a 64-bit pointer offset, to evaluates to an
address far outside the allocated page. Could this result in an out-of-bounds
write if a BPF program provides a maliciously large len?

[Severity: High]
This is another pre-existing issue in bpf_msg_push_data(), but does the
scatterlist element split calculate the offset correctly?

When splitting an element into psge and rsge:

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

Is start the absolute byte offset from the beginning of the entire message?
If the split element is not the first one in the message, wouldn't rsge.offset
be inflated by the cumulative length of all previous elements instead of the
relative offset (start - offset)?

When the network stack eventually reads or writes to this element via sg_virt(),
could this cause an out-of-bounds access past the end of the physical page?

>  	int pop;
>  
>  	if (unlikely(flags))

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