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

Pre-existing issues:
- [Critical] Integer overflow in bpf_msg_push_data leading to out-of-bounds kernel memory write.
- [High] NULL pointer dereference when bpf_msg_pop_data completely empties the message.
- [High] sk_msg_shift_left and sk_msg_shift_right fail to shift the msg->sg.copy bitmap.
- [High] Information leak due to uninitialized memory allocation in bpf_msg_push_data.
--

commit 291c1f747004c017b1fe863b58e41dd47655f42e
Author: Sechang Lim <rhkrqnwk98@gmail.com>

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

start and len are u32, so u64 last = start + len evaluates start + len in
32-bit and wraps before storing it in last. The bounds check can then be
passed with an out-of-range start/len. Widen the addition with a (u64) cast
so the bound is evaluated in 64-bit and a len near U32_MAX no longer wraps.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9590877b0714f..8dac0f4d9fcaf 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2974,8 +2974,8 @@ 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;
> -	int pop;
> +	u64 last = (u64)start + len;
> +	u32 pop;
>  
>  	if (unlikely(flags))
>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but I have a question about the end of this
function. If a BPF program pops the entire message payload (i.e. start == 0
and len == msg->sg.size), the bounds check passes and the pop loop shifts
out all elements, leaving an empty scatterlist.

At the end of bpf_msg_pop_data(), we unconditionally call:

    sk_msg_compute_data_pointers(msg);

Which does:

    struct scatterlist *sge = sk_msg_elem(msg, msg->sg.start);
    ...
    msg->data = sg_virt(sge);

Since the element is now empty, does this cause a NULL pointer dereference
when sg_virt() attempts to dereference the page struct?

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential integer overflow in
the related bpf_msg_push_data() function.

In net/core/filter.c:bpf_msg_push_data():

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

Since len is an unchecked u32 passed from the BPF program, if a very large
len is provided, could copy + len wrap around in 32-bit math and result in
a small memory allocation?

If so, the pointer arithmetic for the destination variable would calculate
an address far beyond the allocated page, potentially causing the memcpy()
to write controlled payload data out of bounds.

[Severity: High]
This isn't a bug introduced by this patch, but looking at the same
allocation in bpf_msg_push_data():

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

It appears to omit the __GFP_ZERO flag. Since only the original front and
back regions are explicitly initialized, does this leave the newly inserted
len bytes uninitialized?

If the BPF program does not manually overwrite these bytes, could
uninitialized kernel memory become part of the packet payload and be
transmitted over the network?

[Severity: High]
This is a pre-existing issue, but while reviewing the scatterlist shifts, I
noticed sk_msg_shift_left() in net/core/filter.c updates the data array:

    do {
        prev = i;
        sk_msg_iter_var_next(i);
        msg->sg.data[prev] = msg->sg.data[i];
    } while (i != msg->sg.end);

However, it doesn't appear to shift the corresponding bits in the
msg->sg.copy bitmap.

Could this desynchronization cause read-only or cloned elements to lose
their copy flag, allowing them to be incorrectly mapped as writable by
sk_msg_compute_data_pointers()?

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