Re: [PATCH bpf-next v3 1/7] bpf, sockmap: reject overflowing copy + len 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] Heap buffer overflow and scatterlist corruption in `bpf_msg_push_data` due to reading a stale length when appending to a full scatterlist ring.
--

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

bpf, sockmap: reject overflowing copy + len in bpf_msg_push_data()

This commit adds an overflow check for the copy + len calculation in the
fallback path of bpf_msg_push_data() to prevent undersized allocations
and subsequent out-of-bounds memcpy operations.

Link: https://lore.kernel.org/all/20260424155913.A19FDC19425@smtp.kernel.org

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..3c8f1cedb217f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2829,6 +2829,9 @@ 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 copy assignment read a stale
length when appending to a full scatterlist ring?

When appending data to a message where start == msg->sg.size, the
scatterlist traversal loop terminates with i == msg->sg.end. If the ring is
full (space == 0) or has one slot left (space == 1), copy is computed using
msg->sg.data[i].length.

Because i == msg->sg.end, this reads the length of an unrelated element
(either the first element if full, or an unused stale slot) instead of the
actual element being split or coalesced.

Later in bpf_msg_push_data(), a memcpy occurs:

	memcpy(raw, from, front);

Can this cause a heap buffer overflow?

The memcpy uses front (which equals the true length of the last element)
to write into the newly allocated page, which is bounded by the unrelated
copy length.

If the stale length read into copy is very small, but the last element is
large, it appears this would write out-of-bounds into the allocation.

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

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