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

On Fri, Aug 7, 2026 at 12:28 PM Pengfei Zhang <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> 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);
>

This is obviously a wrong patch. Please fix  faulty drivers instead of
adding more code in our fast path.

If you use header split, you are supposed to use modern NAPI api, like
napi_gro_frags() which will take care of that.

pw-bot: cr