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

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

[Eric Dumazet]

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.

Use this in geneve for the moment, I suspect we need to adopt this
more broadly.

[1]

BUG: KMSAN: uninit-value in geneve_xmit_skb drivers/net/geneve.c:910 [inline]
 BUG: KMSAN: uninit-value in geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  geneve_xmit_skb drivers/net/geneve.c:910 [inline]
  geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  __netdev_start_xmit include/linux/netdevice.h:4903 [inline]
  netdev_start_xmit include/linux/netdevice.h:4917 [inline]
  xmit_one net/core/dev.c:3531 [inline]
  dev_hard_start_xmit+0x247/0xa20 net/core/dev.c:3547
  __dev_queue_xmit+0x348d/0x52c0 net/core/dev.c:4335
  dev_queue_xmit include/linux/netdevice.h:3091 [inline]
  packet_xmit+0x9c/0x6c0 net/packet/af_packet.c:276
  packet_snd net/packet/af_packet.c:3081 [inline]
  packet_sendmsg+0x8bb0/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

Uninit was created at:
  slab_post_alloc_hook mm/slub.c:3804 [inline]
  slab_alloc_node mm/slub.c:3845 [inline]
  kmem_cache_alloc_node+0x613/0xc50 mm/slub.c:3888
  kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:577
  __alloc_skb+0x35b/0x7a0 net/core/skbuff.c:668
  alloc_skb include/linux/skbuff.h:1318 [inline]
  alloc_skb_with_frags+0xc8/0xbf0 net/core/skbuff.c:6504
  sock_alloc_send_pskb+0xa81/0xbf0 net/core/sock.c:2795
  packet_alloc_skb net/packet/af_packet.c:2930 [inline]
  packet_snd net/packet/af_packet.c:3024 [inline]
  packet_sendmsg+0x722d/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

CPU: 0 PID: 5033 Comm: syz-executor346 Not tainted 6.9.0-rc1-syzkaller-00005-g928a87efa423 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024

Fixes: d13f048dd40e ("net: geneve: modify IP header check in geneve6_xmit_skb and geneve_xmit_skb")
Reported-by: syzbot+9ee20ec1de7b3168db09@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/000000000000d19c3a06152f9ee4@google.com/
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/net/geneve.c     |  4 ++--
 include/net/ip_tunnels.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 2f6739fe78af2e8e90c0a3b474c2e99c83e02994..6c2835086b57eacbcddb44a3c507e26d5a944427 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -822,7 +822,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs4)
@@ -929,7 +929,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs6)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 	return pskb_network_may_pull(skb, nhlen);
 }
 
+/* Strict version of pskb_inet_may_pull().
+ * Once vlan headers are skipped, only accept
+ * ETH_P_IPV6 and ETH_P_IP.
+ */
+static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+	int nhlen, maclen;
+	__be16 type;
+
+	type = __vlan_get_protocol(skb, type, &maclen);
+
+	switch (type) {
+#if IS_ENABLED(CONFIG_IPV6)
+	case htons(ETH_P_IPV6):
+		nhlen = sizeof(struct ipv6hdr);
+		break;
+#endif
+	case htons(ETH_P_IP):
+		nhlen = sizeof(struct iphdr);
+		break;
+
+	default:
+		return 0;
+	}
+	if (pskb_may_pull(skb, maclen + nhlen))
+		return 0;
+
+	skb_set_network_header(skb, maclen);
+	return type;
+}
+
 static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
 {
 	const struct ip_tunnel_encap_ops *ops;
-- 
2.44.0.478.gd926399ef9-goog

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

[Sabrina Dubroca]

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.

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
>  	return pskb_network_may_pull(skb, nhlen);
>  }
>  
> +/* Strict version of pskb_inet_may_pull().
> + * Once vlan headers are skipped, only accept
> + * ETH_P_IPV6 and ETH_P_IP.
> + */
> +static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
> +{
> +	int nhlen, maclen;
> +	__be16 type;

Should that be:

    type = skb->protocol

?

Otherwise it's used uninitialized here:

> +
> +	type = __vlan_get_protocol(skb, type, &maclen);

-- 
Sabrina

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

[Eric Dumazet]

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.

How can we cope properly with this mess ?

>
> > diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> > index 5cd64bb2104df389250fb3c518ba00a3826c53f7..41537d5dce52412e15d7871ec604546582b10098 100644
> > --- a/include/net/ip_tunnels.h
> > +++ b/include/net/ip_tunnels.h
> > @@ -361,6 +361,37 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
> >       return pskb_network_may_pull(skb, nhlen);
> >  }
> >
> > +/* Strict version of pskb_inet_may_pull().
> > + * Once vlan headers are skipped, only accept
> > + * ETH_P_IPV6 and ETH_P_IP.
> > + */
> > +static inline __be16 skb_vlan_inet_prepare(struct sk_buff *skb)
> > +{
> > +     int nhlen, maclen;
> > +     __be16 type;
>
> Should that be:
>
>     type = skb->protocol
>
> ?
>
> Otherwise it's used uninitialized here:
>
> > +
> > +     type = __vlan_get_protocol(skb, type, &maclen);

Arg, a last minute change did not make it.

>
> --
> Sabrina
>

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

[Sabrina Dubroca]

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

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

[Eric Dumazet]

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.

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.

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

[Sabrina Dubroca]

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

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

[Eric Dumazet]

On Thu, Apr 4, 2024 at 7:47 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Apr 4, 2024 at 7:09 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Apr 4, 2024 at 7:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Thu,  4 Apr 2024 13:11:26 +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 mac validation.
> > > >
> > > > Use this in geneve for the moment, I suspect we need to adopt this
> > > > more broadly.
> > >
> > > Something is cause the ttl test do break:
> > >
> > > # │ geneve │     4 │     4 │ inherit 0x3c │    inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
> > > ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
> > >
> > > https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout
> > >
> > > Is is possibly this change?
> >
> > Let me check this, thanks !
>
> Apparently __vlan_get_protocol() depends on skb->mac_len.
>
> This field is not usually set, unless skb_reset_mac_len() has been called.

I will squash tomorrow in v4 this fix, unless someone disagrees.

Thanks again !

diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 6898d0b8306f803c57065f6e134f96dcc8517cad..ec3950be851f2d5a04972222d8cf13b0d811be6f
100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -365,13 +365,14 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
  */
 static inline bool skb_vlan_inet_prepare(struct sk_buff *skb)
 {
-       int nhlen, maclen;
-       __be16 type;
+       int nhlen, maclen = ETH_HLEN;
+       __be16 type = skb->protocol;

        /* Essentially this is skb_protocol(skb, true)
         * And we get MAC len.
         */
-       type = __vlan_get_protocol(skb, skb->protocol, &maclen);
+       if (eth_type_vlan(type))
+               type = __vlan_get_protocol(skb, type, &maclen);

        switch (type) {
 #if IS_ENABLED(CONFIG_IPV6)

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

[Eric Dumazet]

On Thu, Apr 4, 2024 at 7:09 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Apr 4, 2024 at 7:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu,  4 Apr 2024 13:11:26 +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 mac validation.
> > >
> > > Use this in geneve for the moment, I suspect we need to adopt this
> > > more broadly.
> >
> > Something is cause the ttl test do break:
> >
> > # │ geneve │     4 │     4 │ inherit 0x3c │    inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
> > ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
> >
> > https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout
> >
> > Is is possibly this change?
>
> Let me check this, thanks !

Apparently __vlan_get_protocol() depends on skb->mac_len.

This field is not usually set, unless skb_reset_mac_len() has been called.

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

[Eric Dumazet]

On Thu, Apr 4, 2024 at 7:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu,  4 Apr 2024 13:11:26 +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 mac validation.
> >
> > Use this in geneve for the moment, I suspect we need to adopt this
> > more broadly.
>
> Something is cause the ttl test do break:
>
> # │ geneve │     4 │     4 │ inherit 0x3c │    inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
> ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP
>
> https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout
>
> Is is possibly this change?

Let me check this, thanks !

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

[Jakub Kicinski]

On Thu,  4 Apr 2024 13:11:26 +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 mac validation.
> 
> Use this in geneve for the moment, I suspect we need to adopt this
> more broadly.

Something is cause the ttl test do break:

# │ geneve │     4 │     4 │ inherit 0x3c │    inherit 8 │ false │./l2_tos_ttl_inherit.sh: line 350: printf: 0xeaECT0: invalid hex number
ok 1 selftests: net: l2_tos_ttl_inherit.sh # SKIP

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/537382/6-l2-tos-ttl-inherit-sh/stdout

Is is possibly this change?

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

[Eric Dumazet]

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 mac validation.

Use this in geneve for the moment, I suspect we need to adopt this
more broadly.

v2,v3 - Addressed Sabrina comments on v1 and v2
Link: https://lore.kernel.org/netdev/Zg1l9L2BNoZWZDZG@hog/

[1]

BUG: KMSAN: uninit-value in geneve_xmit_skb drivers/net/geneve.c:910 [inline]
 BUG: KMSAN: uninit-value in geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  geneve_xmit_skb drivers/net/geneve.c:910 [inline]
  geneve_xmit+0x302d/0x5420 drivers/net/geneve.c:1030
  __netdev_start_xmit include/linux/netdevice.h:4903 [inline]
  netdev_start_xmit include/linux/netdevice.h:4917 [inline]
  xmit_one net/core/dev.c:3531 [inline]
  dev_hard_start_xmit+0x247/0xa20 net/core/dev.c:3547
  __dev_queue_xmit+0x348d/0x52c0 net/core/dev.c:4335
  dev_queue_xmit include/linux/netdevice.h:3091 [inline]
  packet_xmit+0x9c/0x6c0 net/packet/af_packet.c:276
  packet_snd net/packet/af_packet.c:3081 [inline]
  packet_sendmsg+0x8bb0/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

Uninit was created at:
  slab_post_alloc_hook mm/slub.c:3804 [inline]
  slab_alloc_node mm/slub.c:3845 [inline]
  kmem_cache_alloc_node+0x613/0xc50 mm/slub.c:3888
  kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:577
  __alloc_skb+0x35b/0x7a0 net/core/skbuff.c:668
  alloc_skb include/linux/skbuff.h:1318 [inline]
  alloc_skb_with_frags+0xc8/0xbf0 net/core/skbuff.c:6504
  sock_alloc_send_pskb+0xa81/0xbf0 net/core/sock.c:2795
  packet_alloc_skb net/packet/af_packet.c:2930 [inline]
  packet_snd net/packet/af_packet.c:3024 [inline]
  packet_sendmsg+0x722d/0x9ef0 net/packet/af_packet.c:3113
  sock_sendmsg_nosec net/socket.c:730 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:745
  __sys_sendto+0x685/0x830 net/socket.c:2191
  __do_sys_sendto net/socket.c:2203 [inline]
  __se_sys_sendto net/socket.c:2199 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2199
 do_syscall_64+0xd5/0x1f0
 entry_SYSCALL_64_after_hwframe+0x6d/0x75

CPU: 0 PID: 5033 Comm: syz-executor346 Not tainted 6.9.0-rc1-syzkaller-00005-g928a87efa423 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024

Fixes: d13f048dd40e ("net: geneve: modify IP header check in geneve6_xmit_skb and geneve_xmit_skb")
Reported-by: syzbot+9ee20ec1de7b3168db09@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/000000000000d19c3a06152f9ee4@google.com/
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Phillip Potter <phil@philpotter.co.uk>
Cc: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/geneve.c     |  4 ++--
 include/net/ip_tunnels.h | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 2f6739fe78af2e8e90c0a3b474c2e99c83e02994..6c2835086b57eacbcddb44a3c507e26d5a944427 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -822,7 +822,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs4)
@@ -929,7 +929,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	__be16 sport;
 	int err;
 
-	if (!pskb_inet_may_pull(skb))
+	if (!skb_vlan_inet_prepare(skb))
 		return -EINVAL;
 
 	if (!gs6)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 5cd64bb2104df389250fb3c518ba00a3826c53f7..6898d0b8306f803c57065f6e134f96dcc8517cad 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -361,6 +361,41 @@ static inline bool pskb_inet_may_pull(struct sk_buff *skb)
 	return pskb_network_may_pull(skb, nhlen);
 }
 
+/* Variant of pskb_inet_may_pull().
+ */
+static inline bool skb_vlan_inet_prepare(struct sk_buff *skb)
+{
+	int nhlen, maclen;
+	__be16 type;
+
+	/* Essentially this is skb_protocol(skb, true)
+	 * And we get MAC len.
+	 */
+	type = __vlan_get_protocol(skb, skb->protocol, &maclen);
+
+	switch (type) {
+#if IS_ENABLED(CONFIG_IPV6)
+	case htons(ETH_P_IPV6):
+		nhlen = sizeof(struct ipv6hdr);
+		break;
+#endif
+	case htons(ETH_P_IP):
+		nhlen = sizeof(struct iphdr);
+		break;
+
+	default:
+		nhlen = 0;
+	}
+	/* For ETH_P_IPV6/ETH_P_IP we make sure to pull
+	 * a base network header in skb->head.
+	 */
+	if (!pskb_may_pull(skb, maclen + nhlen))
+		return false;
+
+	skb_set_network_header(skb, maclen);
+	return true;
+}
+
 static inline int ip_encap_hlen(struct ip_tunnel_encap *e)
 {
 	const struct ip_tunnel_encap_ops *ops;
-- 
2.44.0.478.gd926399ef9-goog