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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-04-03, 16:59:52 +0200, Eric Dumazet wrote:
> On Wed, Apr 3, 2024 at 4:55 PM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 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?
> 
> Please read my changelog, I explained that skb_protocol(skb, true) is
> parsing the Ethernet header up to the non vlan proto.

I meant in the ARP/MACsec/whatever else case, using ip_hdr is ugly but
won't do anything wrong.

> syzbot buillt a vlan packet with final proto being IPv4.
> 
> So the helpers who are using skb_protocol() do not understand the IP
> header has not been pulled.
> 
> >
> > 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?
> 
> If you look at the syzbot report, the ip header is definitely dereferenced.

Sure, something is clearly needed to fix what syzbot found (and
pulling more headers is the reasonable way to deal with it).

But AFAICT, if the skb inner contents are not IP/IPv6, the IP header
won't get looked at, even if we have ip_hdr() calls everywhere. That's
what I tried to say with:

    those helpers only look at their iph argument when
    skb_protocol(skb, true) is ETH_P_IP or ETH_P_IPV6

-- 
Sabrina