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

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

[Jiayuan Chen]

Kenneth Lee reported a bug [1]:

  skb_vlan_push got skb with skb->data not at mac header (offset 14)
  WARNING: net/core/skbuff.c:6474 at skb_vlan_push+0x5ec/0x8a0
   ____bpf_skb_vlan_push net/core/filter.c:3239 [inline]
   bpf_skb_vlan_push+0x216/0x8b0 net/core/filter.c:3229
   bpf_prog_run_data_pointers+0x17c/0x240 include/linux/filter.h:917
   tcf_bpf_act+0x31c/0x940 net/sched/act_bpf.c:50
   tcf_action_exec+0x2c1/0x720 net/sched/act_api.c:1147
   tcf_classify+0x6e4/0x1080 net/sched/cls_api.c:1860
   tc_run+0x31c/0x5d0 net/core/dev.c:4411
   sch_handle_ingress net/core/dev.c:4486 [inline]
   __netif_receive_skb_core+0x141b/0x2ec0 net/core/dev.c:6054

bpf_prog_run_generic_xdp() just assumes xdp->data is an Ethernet header:

	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;

But an L3 device (ARPHRD_NONE, ARPHRD_TUNNEL, ...) has no L2 header at
all - its mac_len is 0 and xdp->data is really the L3 header, so those
orig_* values are garbage.

When that garbage comparison says the header changed, we then do:

	__skb_push(skb, ETH_HLEN);
	skb->pkt_type = PACKET_HOST;
	skb->protocol = eth_type_trans(skb, skb->dev);

eth_type_trans() resets mac_header and pulls ETH_HLEN back, so skb->data
does not move at all, but we now have mac_len == 0 while
skb_mac_header(skb) == skb->data - ETH_HLEN.

tc ingress relies on mac_len == skb->data - skb_mac_header(skb) to get
skb->data onto the mac header, so __skb_push(skb, skb->mac_len) pushes
nothing, the program runs with skb->data ETH_HLEN past the mac header,
and bpf_skb_vlan_push() hits the WARN_ONCE above.  skb->protocol also
ends up parsed from uninitialised headroom.

So skip the Ethernet part entirely when the skb has no ETH_HLEN sized L2
header.  The program still runs and its action is still returned.

Refusing to attach XDP to L3 devices instead would not help: mac_len is a
property of the skb, not of the netdev, so an ARPHRD_ETHER device can see
mac_len == 0 too.

[1]: https://lore.kernel.org/bpf/20260812043643.808295-1-kennethbwlee@snu.ac.kr/

Fixes: 22b6034323fd ("net, xdp: Update pkt_type if generic XDP changes unicast MAC")
Reported-by: Kenneth Lee <kennethbwlee@snu.ac.kr>
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 net/core/dev.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b390c2edfb33..5c395460854f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5489,11 +5489,12 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 {
 	void *orig_data, *orig_data_end, *hard_start;
 	struct netdev_rx_queue *rxqueue;
-	bool orig_bcast, orig_host;
+	bool orig_bcast = false, orig_host = false;
+	__be16 orig_eth_type = 0;
 	u32 mac_len, frame_sz;
-	__be16 orig_eth_type;
 	struct ethhdr *eth;
 	u32 metalen, act;
+	bool has_eth;
 	int off;
 
 	/* The XDP program wants to see the packet starting at the MAC
@@ -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;
-	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;
+
+	/* 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);
 
@@ -5554,14 +5566,16 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
 		skb->data_len = 0;
 
 	/* check if XDP changed eth hdr such SKB needs update */
-	eth = (struct ethhdr *)xdp->data;
-	if ((orig_eth_type != eth->h_proto) ||
-	    (orig_host != ether_addr_equal_64bits(eth->h_dest,
-						  skb->dev->dev_addr)) ||
-	    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
-		__skb_push(skb, ETH_HLEN);
-		skb->pkt_type = PACKET_HOST;
-		skb->protocol = eth_type_trans(skb, skb->dev);
+	if (has_eth) {
+		eth = (struct ethhdr *)xdp->data;
+		if ((orig_eth_type != eth->h_proto) ||
+		    (orig_host != ether_addr_equal_64bits(eth->h_dest,
+							  skb->dev->dev_addr)) ||
+		    (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) {
+			__skb_push(skb, ETH_HLEN);
+			skb->pkt_type = PACKET_HOST;
+			skb->protocol = eth_type_trans(skb, skb->dev);
+		}
 	}
 
 	/* Redirect/Tx gives L2 packet, code that will reuse skb must __skb_pull
-- 
2.43.0

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

[sashiko-bot]

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 <jiayuan.chen@linux.dev>

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/20260812043643.808295-1-kennethbwlee@snu.ac.kr/

> 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/20260813113229.381136-1-jiayuan.chen@linux.dev?part=1