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

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

Pre-existing issues:
- [High] Preexisting bug: bpf_xdp_adjust_tail for generic XDP incorrectly frees system_page_pool fragment pages using the driver's RX queue memory model.
- [High] The exact length accounting vulnerability fixed in `bpf_prog_run_generic_xdp` remains unpatched in the native XDP receive path for veth devices (`veth_xdp_rcv_skb`).
--

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

Generic XDP exposes non-linear skb fragments through an xdp_buff. If an
XDP program adjusts the fragment area, bpf_prog_run_generic_xdp() copies
xdp_frags_size back to skb->data_len but leaves skb->len containing the
old fragment contribution.

This patch subtracts the old data_len before replacing it and adds the new
data_len afterwards, keeping skb->len and skb->data_len 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

[Severity: High]
This is a pre-existing issue, but since this code path relates to fragment
shrinking via bpf_xdp_adjust_tail(), does it incorrectly free system_page_pool
fragment pages using the driver's RX queue memory model?

In generic XDP, skb fragments are reallocated using system_page_pool:

net/core/dev.c:netif_skb_check_for_xdp() {
    ...
    err = skb_cow_data_for_xdp(this_cpu_read(system_page_pool.pool), pskb, prog);
    ...
}

However, bpf_prog_run_generic_xdp() initializes the xdp_buff with the device
driver's rxqueue:

net/core/dev.c:bpf_prog_run_generic_xdp() {
    ...
    xdp_init_buff(xdp, frame_sz, &rxqueue->xdp_rxq);
    ...
}

When an XDP program shrinks fragments, bpf_xdp_shrink_data() retrieves the
driver's mem_type and passes it to __xdp_return():

net/core/filter.c:bpf_xdp_shrink_data() {
    ...
    enum xdp_mem_type mem_type = xdp->rxq->mem.type;
    ...
    if (release) {
        __xdp_return(netmem, mem_type, false, zc_frag);
    ...
}

If mem_type is MEM_TYPE_PAGE_SHARED, __xdp_return() routes to page_frag_free(),
bypassing the required page_pool_put_full_netmem() hooks for system_page_pool
pages. Can this leak DMA mappings and corrupt page structures when these
pages reach the buddy allocator?

> -	 * (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;
> +	if (xdp_buff_has_frags(xdp)) {
>  		skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> -	else
> +		skb->len += skb->data_len;

[Severity: High]
This isn't a bug introduced by this patch, but does the exact length
accounting vulnerability fixed here remain unpatched in the native XDP
receive path for veth devices?

Looking at veth_xdp_rcv_skb() in drivers/net/veth.c:

drivers/net/veth.c:veth_xdp_rcv_skb() {
    ...
    off = xdp->data_end - orig_data_end;
    if (off != 0)
        __skb_put(skb, off); /* positive on grow, negative on shrink */

    /* 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.
     */
    if (xdp_buff_has_frags(xdp))
        skb->data_len = skb_shinfo(skb)->xdp_frags_size;
    else
        skb->data_len = 0;
    ...
}

If an XDP program shrinks an skb's fragment area, skb->data_len is updated
but skb->len is not adjusted, which would artificially inflate skb_headlen()
and potentially leak adjacent kernel memory to userspace.

Additionally, if the fragments are completely dropped, xdp->data_end shrinks,
resulting in a negative off passed to __skb_put(). Since skb->data_len is
still non-zero during the __skb_put() call, would SKB_LINEAR_ASSERT() trigger
a BUG_ON and cause a kernel panic?

> +	} 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