| Message ID | 20240404131126.2534400-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 |
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?
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 !
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.
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)
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;
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(-)