Re: [PATCH net v2 1/2] net: fix skb length accounting after generic XDP frag adjustment

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

Pre-existing issues:
- [Critical] The exact same metadata desynchronization bug that causes OOB reads in generic XDP also exists in drivers/net/veth.c, including an additional kernel panic vulnerability via BUG_ON.
- [Critical] bpf_xdp_frags_shrink_tail() incorrectly frees system_page_pool pages using page_frag_free() for Generic XDP.
- [High] Preexisting BUG_ON() panic in generic XDP when the head is pulled by more than the original linear length.
--

--- Patch [1]: [PATCH net v2 1/2] net: fix skb length accounting after generic XDP frag adjustment ---
commit 98b02c083e22c12a3a47fab8bce5b83fb6fad2b7
Author: Sun Jian <[email protected]>

net: fix skb length accounting after generic XDP frag adjustment

This commit fixes an out-of-bounds read caused by generic XDP fragment
adjustment. It subtracts the old data_len from skb->len before updating
it to the new fragment size, keeping the lengths synchronized.

Link: https://lore.kernel.org/bpf/al9T9Eto%2FhRIzP5W@boxer/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5933c5dab09ee..5c37cf6c4aa10 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5517,12 +5517,16 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	}
>  
>  	/* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
> -	 * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
> +	 * (e.g. bpf_xdp_adjust_tail). Remove the old fragment contribution
> +	 * from skb->len before updating data_len, then add the new one back.
>  	 */
> -	if (xdp_buff_has_frags(xdp))
> +	skb->len -= skb->data_len;

[Severity: High]
This is a pre-existing issue, but does bpf_prog_run_generic_xdp() still
panic if the head is pulled by more than the original linear length?

If an XDP program shrinks a packet's fragments to 0, it can subsequently
grow the linear part and pull the head pointer (via bpf_xdp_adjust_head)
by an amount greater than the original linear length.

In bpf_prog_run_generic_xdp(), the head adjustment is processed before
the tail and data_len updates:

        if (off > 0)
                __skb_pull(skb, off);

Since __skb_pull() asserts that skb->len >= skb->data_len and skb->data_len
still holds its original value, wouldn't skb->len -= off fall below
skb->data_len and trigger a kernel panic?

[Severity: Critical]
This is a pre-existing issue, but does veth_xdp_rcv_skb() in
drivers/net/veth.c have the exact same metadata desynchronization bug?

If a fragmented packet is processed by a tail-adjusting XDP program in
veth_xdp_rcv_skb(), skb->len is unmodified while skb->data_len changes:

        /* check if bpf_xdp_adjust_tail was used */
        off = xdp->data_end - orig_data_end;
        if (off != 0)
                __skb_put(skb, off); /* positive on grow, negative on shrink */

If the packet is shrunk enough to remove all fragments and reduce the linear
part, wouldn't calling __skb_put(skb, off) with a negative offset trigger a
BUG_ON since skb->data_len is still non-zero at this point?

[Severity: Critical]
This is a pre-existing issue, but does bpf_xdp_shrink_data() incorrectly
free system_page_pool pages using page_frag_free() for Generic XDP?

When generic XDP processes a non-linear SKB, fragments are allocated from
system_page_pool. However, xdp_buff is initialized using the driver's
rxqueue->xdp_rxq (often MEM_TYPE_PAGE_SHARED).

If an XDP program shrinks the packet tail, bpf_xdp_shrink_data() drops
fragments and extracts the driver's mem_type:

        enum xdp_mem_type mem_type = xdp->rxq->mem.type;
        ...
        if (release) {
                __xdp_return(netmem, mem_type, false, zc_frag);
        }

Because the memory type relies on the driver's default, wouldn't
__xdp_return() incorrectly use page_frag_free() instead of returning the
page to the page_pool?

        case MEM_TYPE_PAGE_SHARED:
                page_frag_free(__netmem_address(netmem));
                break;

Could this bypass the page_pool recycling logic and free a page_pool page
directly to the buddy allocator, triggering a bad_page() BUG?

> +	if (xdp_buff_has_frags(xdp)) {
>  		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> -	else
> +		skb->len += skb->data_len;
> +	} else {
>  		skb->data_len = 0;
> +	}
>  
>  	/* check if XDP changed eth hdr such SKB needs update */
>  	eth = (struct ethhdr *)xdp->data;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1