| Message ID | 20240403113853.3877116-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] geneve: fix header validation in geneve[6]_xmit_skb | expand |
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);
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 >
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?
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.
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
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;
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(-)