Re: [PATCH net 0/5] net: fix eth_type_trans() crashes on skbs coming from the stack

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Eric Dumazet wrote:
> eth_type_trans() assumes ETH_HLEN bytes are present in skb->head.
> NIC drivers always provide this, and they should keep using the fast
> version, without an extra pskb_may_pull().
> 
> Devices looping skbs from their transmit path back to their receive
> path get skbs built by the stack instead. tc actions (act_vlan pop_eth,
> act_ife decode), BPF helpers (bpf_skb_change_head() before a redirect)
> or OVS actions (pop_eth followed by pop_mpls/pop_nsh) can leave fewer
> than ETH_HLEN bytes in the linear part, and eth_type_trans() then hits
> the BUG() in __skb_pull().
> 
> Instead of fixing every producer, or adding a check to all
> eth_type_trans() users, fix the few consumers exposed to this:

Definitely a nice approach.

You previously suggested catching most of the TC modifications with a
single check in sch_handle_(e|in)gress. Is that no longer needed?

The few mirred/redirect cases are a bane, causing so many issues. And
escape any such check, as they call netif_receive_skb, netif_rx,
dev_queue_xmit, directly.

If it's only act_mirred, bpf_redirect and OVS, maybe a single
validation helper across all three before they pass to the rx/tx
path, that for starters calls eth_type_trans_careful. It quite likely
won't be the last packet corruption they are capable of..

> 
> - Patch 1 adds eth_type_trans_careful(), which pulls the Ethernet
>   header first and returns a drop reason on failure.
> - Patch 2 uses it in loopback_xmit().
> - Patch 3 uses it in __dev_forward_skb2(), used by BPF redirects to
>   ingress and by veth, macvlan, ipvlan, netdevsim and l2tp_eth.
> - Patch 4 uses it in OVS internal_dev_recv(), covering both the MPLS
>   (TEB) and NSH pop cases at once (only the MPLS case was tested).
> - Patch 5 (from Jamal) fixes act_ife decode on egress, which was
>   consuming the inner Ethernet header. This is no longer a crash after
>   patch 2, but egress decode never delivered a correct frame.
> 
> Other eth_type_trans() users already pull the header or never see
> such skbs:
> - vrf_process_v{4,6}_outbound() already pull ETH_HLEN + IP header
>   before vrf_local_xmit().
> - tun already uses pskb_may_pull_reason(skb, ETH_HLEN).
> - XDP: bpf_xdp_adjust_head() and bpf_xdp_adjust_tail() keep at least
>   ETH_HLEN bytes in the linear part, and multi-buffer tail shrinking
>   only trims frags.
> 
> Tested under virtme-ng with AF_PACKET reproducers. Without the series,
> all four cases crash in eth_type_trans():
> - lo egress "action vlan pop_eth", eth/eth/8192 bytes frame
> - lo egress "action ife decode pipe", IFE frame with 8192 bytes payload
> - lo egress "action vlan pop_eth continue", then a BPF program doing
>   bpf_skb_change_head(skb, 1, 0) + bpf_redirect(veth1, BPF_F_INGRESS)
> - OVS flow "pop_eth,pop_mpls(0x6558),0" to the internal port,
>   eth/mpls/eth/4096 bytes frame
> With the series, no crash. An ICMP echo inside an IFE frame decoded
> on lo egress is now received and answered. Normal traffic on lo,
> veth (including 8000 byte pings) and vrf is unchanged.
> 
> Link: https://lore.kernel.org/netdev/20261005052249.1914367-2-weir@nebusec.ai/
> Link: https://lore.kernel.org/netdev/ACT-N6N4.v3.r0.20261023200137-4@mojatatu.com/
> 
> Eric Dumazet (4):
>   net: add eth_type_trans_careful()
>   loopback: use eth_type_trans_careful()
>   net: use eth_type_trans_careful() in __dev_forward_skb2()
>   openvswitch: use eth_type_trans_careful() in internal_dev_recv()
> 
> Jamal Hadi Salim (1):
>   net/sched: act_ife: do not consume the inner header on egress
> 
>  drivers/net/loopback.c               |  8 +++++++-
>  include/linux/etherdevice.h          | 28 ++++++++++++++++++++++++++++
>  net/core/dev.c                       | 18 +++++++++++++-----
>  net/openvswitch/vport-internal_dev.c |  8 +++++++-
>  net/sched/act_ife.c                  |  4 ++++
>  5 files changed, 59 insertions(+), 7 deletions(-)
> 
> -- 
> 2.53.0
>