Re: [PATCH net-next] net: xdp: don't assume an Ethernet header in generic XDP

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

Pre-existing issues:
- [High] Incorrect SKB length and tail pointer calculation after generic XDP tail adjustment.
--

--- Patch [1]: [PATCH net-next] net: xdp: don't assume an Ethernet header in generic XDP ---
commit 3332d9a3f6b94e19ff995230c0e36bef6be765df
Author: Jiayuan Chen <[email protected]>

net: xdp: don't assume an Ethernet header in generic XDP
    
This patch fixes generic XDP for L3 devices (like tun/tap or ARPHRD_NONE)
that do not have an Ethernet header. It skips Ethernet-specific checks and
header adjustments when the SKB does not carry an ETH_HLEN sized L2 header.

Link: https://lore.kernel.org/bpf/[email protected]/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index fd0b445f5d38c..81de25a2b421d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5519,10 +5520,21 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  
>  	orig_data_end = xdp->data_end;
>  	orig_data = xdp->data;
> +
> +	/* xdp->data only points at an Ethernet header if this skb actually
> +	 * carries one.  Devices with a different link layer (mac_len == 0
> +	 * for ARPHRD_NONE/TUNNEL/RAWIP/PPP/..., IPOIB_ENCAP_LEN for IPoIB)
> +	 * have nothing to inspect here, and must not have skb->mac_header
> +	 * relocated by the ETH_HLEN fixup below.
> +	 */
> +	has_eth = mac_len == ETH_HLEN;
> +	if (has_eth) {
> +		eth = (struct ethhdr *)xdp->data;
> +		orig_host = ether_addr_equal_64bits(eth->h_dest,
> +						    skb->dev->dev_addr);
> +		orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest);
> +		orig_eth_type = eth->h_proto;
> +	}
>  
>  	act = bpf_prog_run_xdp(xdp_prog, xdp);
>  

[Severity: High]
This isn't a bug introduced by this patch, but looking at how xdp->data and
skb->data relate to the MAC header, could there be a tail pointer calculation
issue when bpf_xdp_adjust_tail() is called?

Before bpf_prog_run_generic_xdp() executes, the MAC header is stripped via
__skb_pull(skb, mac_len), leaving skb->data pointing at the network layer.
However, xdp->data is initialized to point at the MAC header.

If the XDP program adjusts the tail, the new tail pointer is updated further
down in bpf_prog_run_generic_xdp():

	/* check if bpf_xdp_adjust_tail was used */
	off = xdp->data_end - orig_data_end;
	if (off != 0) {
		skb_set_tail_pointer(skb, xdp->data_end - xdp->data);
		skb->len += off;
	}

Because xdp->data_end - xdp->data includes the MAC header length, but
skb_set_tail_pointer() applies this offset relative to skb->data (which is
already at the network header), doesn't this push the tail pointer mac_len
bytes too far past the actual end of the packet?

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