Re: [PATCH bpf v2 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 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] bpf_msg_push_data and bpf_msg_pull_data lack a definitive `start <= msg->sg.size` validation. When operating on an empty or shrunk message, the do-while loop executes at least once and accesses the unused slot at `msg->sg.start`. This slot may contain a 'ghost' element from a previous `sk_msg_recvmsg` consumption, retaining a non-zero length and a freed page pointer. The lack of a `msg->sg.size` check allows a crafted `start` offset to bypass the bounds check, leading to a severe use-after-free.
- [High] Appending data to a full scatterlist ring bypasses the fallback path and corrupts the ring state. When `space == 0` and the append offset matches the end of the message, `bpf_msg_push_data` reads `copy = msg->sg.data[msg->sg.end].length`. If this unused slot is zero, the `if (copy)` fallback block is skipped. The function unconditionally increments `msg->sg.end`, forcing it to wrap around and equal `msg->sg.start`. This breaks the ring buffer state, causing `sk_msg_elem_used()` to return 0, falsely emptying the ring and leaking all previously allocated pages.
--

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

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

This commit adds an overflow check to prevent copy + len from wrapping
around when allocating a page in bpf_msg_push_data. This fixes an issue
where an undersized allocation could lead to an out-of-bounds memcpy.

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

[Severity: Critical]
This is a pre-existing issue, but does bpf_msg_push_data lack a definitive
start <= msg->sg.size validation?

When operating on an empty or shrunk message, the do-while loop executes
at least once and accesses the unused slot at msg->sg.start:

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

    if (start > offset + l)
        return -EINVAL;

If sk_msg_recvmsg advances msg->sg.start and frees the page without
zeroing the element length or pointer, could this helper read a stale
ghost element?

Without a msg->sg.size validation, could the helper incorrectly accept the
bounds as valid, leading to memcpy operations on a freed page pointer?

[Severity: High]
This is a pre-existing issue, but does appending data to a full
scatterlist ring bypass the fallback path and corrupt the ring state?

When space is 0 and the append offset matches the end of the message,
bpf_msg_push_data reads copy = msg->sg.data[msg->sg.end].length:

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

If this unused slot is zero, the if (copy) fallback block is skipped:

    if (copy) {
        ...
        goto place_new;
    }

    ...

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

Does this unconditionally increment msg->sg.end, forcing it to wrap around
and equal msg->sg.start?

If so, could this break the ring buffer state, causing sk_msg_elem_used to
return 0, falsely emptying the ring and leaking all previously allocated
pages?

[Severity: Critical]
This is a pre-existing issue, but does a similar logic apply to
bpf_msg_pull_data?

    do {
        offset += len;
        len = sk_msg_elem(msg, i)->length;
        if (start < offset + len)
            break;
        sk_msg_iter_var_next(i);
    } while (i != msg->sg.end);

    if (unlikely(start >= offset + len))
        return -EINVAL;

Could this fail to validate against the actual message size and proceed
to process freed scatterlist elements?

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