Re: [PATCH net] geneve: fix header validation in geneve[6]_xmit_skb

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-03, 16:25:47 +0200, Eric Dumazet wrote:
> On Wed, Apr 3, 2024 at 4:21 PM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2024-04-03, 11:38:53 +0000, Eric Dumazet wrote:
> > > syzbot is able to trigger an uninit-value in geneve_xmit() [1]
> > >
> > > Problem : While most ip tunnel helpers (like ip_tunnel_get_dsfield())
> > > uses skb_protocol(skb, true), pskb_inet_may_pull() is only using
> > > skb->protocol.
> > >
> > > If anything else than ETH_P_IPV6 or ETH_P_IP is found in skb->protocol,
> > > pskb_inet_may_pull() does nothing at all.
> > >
> > > If a vlan tag was provided by the caller (af_packet in the syzbot case),
> > > the network header might not point to the correct location, and skb
> > > linear part could be smaller than expected.
> > >
> > > Add skb_vlan_inet_prepare() to perform a complete validation and pull.
> > > If no IPv4/IPv6 header is found, it returns 0.
> >
> > And then geneve_xmit_skb/geneve6_xmit_skb drops the packet, which
> > breaks ARP over a geneve tunnel, and other valid things like macsec.
> 
> geneve_xmit_skb() uses ip_hdr() blindly.

Do those actually end up getting used? They get passed to
{ip_tunnel_ecn_encap,ip_tunnel_get_ttl,ip_tunnel_get_dsfield}, and
those helpers only look at their iph argument when skb_protocol(skb,
true) is ETH_P_IP or ETH_P_IPV6. So, definitely not pretty, but I
don't see a bug there. Am I missing something?

From a quick look, most users of those helpers seem to pass
ip_hdr(skb) (except for ip_tunnel_ecn_encap called from
ip_md_tunnel_xmit and ip_tunnel_xmit -- vxlan_xmit_one uses a cached
version but I don't think it's needed). Would it be less confusing if
we removed that argument and let the helper fetch ip_hdr?

-- 
Sabrina