[PATCH net-next] net: ethernet: drop skbs with a short linear part in eth_type_trans()

[Pengfei Zhang]

eth_type_trans() pulls ETH_HLEN unconditionally, but never checks that the
skb has that many bytes in its linear part.  Only skb->len is checked, by
skb_pull_inline(), while the __skb_pull() it calls requires
len <= skb_headlen(skb).  The two are the same test for a linear skb and
differ for a paged one.

Drivers using hardware header split, such as amd-xgbe and dwc-xlgmac, can
end up with a paged skb whose linear part is shorter than the header: on
certain frames the controller reports a header length below ETH_HLEN.  With
2, that gives len=56, headlen=2 and data_len=54.  Nothing is wrong with
that skb, but eth_type_trans() on it hits the BUG() in __skb_pull() from
softirq context, so one received packet panics the machine.

Drop the frame instead.  Test skb_headlen() rather than skb->len, since
that is what __skb_pull() requires.  Returning 0 leaves skb->protocol
unset, so the frame is counted in rx_dropped and freed with
SKB_DROP_REASON_UNHANDLED_PROTO rather than discarded silently.

Signed-off-by: Pengfei Zhang <zhangfeionline@xxxxxxxxx>
---
Notes for reviewers, not for the changelog:

We ran into this on a platform built on the Synopsys DesignWare Core IP,
where a single received frame is enough to panic the machine.  Nothing was
corrupted -- the skb was intact and every invariant held.  What is missing
is a check that the header being pulled is actually present.

The length in question is reported by the MAC itself: with header split
enabled it writes into the receive descriptor how far into the frame it cut
the header.  A MAC reporting less than ETH_HLEN there is misbehaving, and
that much is the hardware's problem.  But eth_type_trans() requires
ETH_HLEN linear bytes and tests only skb->len, so the short length reaches
the BUG() in softirq context, on a path fed by received traffic.

The same failure mode was CVE-2024-41091 when it was reachable through
tun_xdp_one(), and was fixed there by dropping the frame, in 049584807f1d
("tun: add missing verification for short frame").

amd-xgbe shows the same shape in-tree:

  /* On some frames the MAC reports a header length below ETH_HLEN in the
   * receive descriptor.  The driver takes that value as-is; nothing bounds
   * it from below.
   */
  xgbe-dev.c:1901
        rdata->rx.hdr_len = XGMAC_GET_BITS_LE(rdesc->desc2,
                                             RX_NORMAL_DESC2, HL);

  /* The length is passed down unchanged and used to fill the skb, through
   * the ordinary core helpers.  The numbers below are one instance of it,
   * hdr_len = 2 on a 56-byte frame:
   */
  xgbe-drv.c:2355
        skb = xgbe_create_skb(pdata, napi, rdata, buf1_len);
            napi_alloc_skb(napi, rdata->rx.hdr.dma_len)
            skb_copy_to_linear_data(skb, packet, len)    /* len = 2 */
            skb_put(skb, len)                            /* headlen = 2 */

  xgbe-drv.c:2370
        skb_add_rx_frag(...)      /* the other 54 bytes, as a frag */

  /* The skb is well formed here -- 56 == 2 + 54 -- and eth_type_trans()
   * pulls the MAC header without testing that it is in the linear part.
   * The patch adds that test in eth_type_trans(), just before the pull
   * marked below.  Without it the pull takes the machine down:
   */
  xgbe-drv.c:2435
        skb->protocol = eth_type_trans(skb, netdev);
            eth_skb_pull_mac(skb)              <- eth.c:164, unguarded
                skb_pull_inline(skb, ETH_HLEN)      /* 14 > 56, false */
                    __skb_pull(skb, ETH_HLEN)
                        skb->len -= len;            /* 56 - 14 = 42 */
                        if (skb->len < skb->data_len)   /* 42 < 54 */
                                BUG();              /* fatal in softirq */

Every step there uses the standard core APIs, and there is no skb memory
corruption anywhere along the way: the BUG() fires on arithmetic that
__skb_pull() just did itself.  A driver doing nothing unusual walks past
the one test there is and into it.  A received packet should not be able
to make the stack panic on purpose, so the check belongs where the
requirement is -- the stack has everything it needs to reject the frame
itself, and should not have to rely on the hardware or the driver
reporting a sane length.

The BUG() in __skb_pull() is deliberately left alone.  It is the backstop
for the many skb_pull() call sites that discard the return value, and for
real corruption it should stay a panic.

 net/ethernet/eth.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index d9faadbe9..84914709a 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -161,6 +161,12 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 	skb->dev = dev;
 	skb_reset_mac_header(skb);
 
+	if (unlikely(skb_headlen(skb) < ETH_HLEN)) {
+		net_warn_ratelimited("%s: dropping frame with a short linear part from %s\n",
+				     __func__, dev->name);
+		return 0;
+	}
+
 	eth = eth_skb_pull_mac(skb);
 	eth_skb_pkt_type(skb, dev);
 

base-commit: 4fa4977a0d900f936bcae5cd2c510be5554e8dd6
-- 
2.54.0