Skip to contentSkip to conversation

udp: disable gso with no_check_tx

PATCH Applied Willem de Bruijn 3 participants 4 messages Apr 30, 2018 – May 2, 2018

Subsystems (3)
Networking Network drivers UDP
lore
Share & tools
lore

Willem de Bruijn

author maintainer
#0 lore raw

PATCH 1/1 udp: disable gso with no_check_tx

From: Willem de Bruijn <[email protected]>

Syzbot managed to send a udp gso packet without checksum offload into the gso stack by disabling tx checksum (UDP_NO_CHECK6_TX). This triggered the skb_warn_bad_offload.

  RIP: 0010:skb_warn_bad_offload+0x2bc/0x600 net/core/dev.c:2658
   skb_gso_segment include/linux/netdevice.h:4038 [inline]
   validate_xmit_skb+0x54d/0xd90 net/core/dev.c:3120
   __dev_queue_xmit+0xbf8/0x34c0 net/core/dev.c:3577
   dev_queue_xmit+0x17/0x20 net/core/dev.c:3618

UDP_NO_CHECK6_TX sets skb->ip_summed to CHECKSUM_NONE just after the udp gso integrity checks in udp_(v6_)send_skb. Extend those checks to catch and fail in this case.

After the integrity checks jump directly to the CHECKSUM_PARTIAL case to avoid reading the no_check_tx flags again (a TOCTTOU race).

Fixes bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
Signed-off-by Willem de Bruijn <[email protected]>
---
 net/ipv4/udp.c | 4 ++++
 net/ipv6/udp.c | 4 ++++
 2 files changed, 8 insertions(+)
net/ipv4/udp.c +4 -0
index 794aeafeb782..dd3102a37ef9 100644
@@ -786,11 +786,14 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 			return -EINVAL;
 		if (skb->len > cork->gso_size * UDP_MAX_SEGMENTS)
 			return -EINVAL;
+		if (sk->sk_no_check_tx)
+			return -EINVAL;
 		if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite)
 			return -EIO;
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
+		goto csum_partial;
 	}
 
 	if (is_udplite)  				 /*     UDP-Lite      */

@@ -802,6 +805,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 		goto send;
 
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) { /* UDP hardware csum */
+csum_partial:
 
 		udp4_hwcsum(skb, fl4->saddr, fl4->daddr);
 		goto send;
net/ipv6/udp.c +4 -0
index 6acfdd3e442b..a34e28ac03a7 100644
@@ -1051,11 +1051,14 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 			return -EINVAL;
 		if (skb->len > cork->gso_size * UDP_MAX_SEGMENTS)
 			return -EINVAL;
+		if (udp_sk(sk)->no_check6_tx)
+			return -EINVAL;
 		if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite)
 			return -EIO;
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
+		goto csum_partial;
 	}
 
 	if (is_udplite)

@@ -1064,6 +1067,7 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 		skb->ip_summed = CHECKSUM_NONE;
 		goto send;
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) { /* UDP hardware csum */
+csum_partial:
 		udp6_hwcsum_outgoing(sk, skb, &fl6->saddr, &fl6->daddr, len);
 		goto send;
 	} else
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
maintainer Applied
#1 lore raw

From: Willem de Bruijn <[email protected]>
Date: Mon, 30 Apr 2018 15:58:36 -0400

Quoted 4 lines

From: Willem de Bruijn <[email protected]>

Syzbot managed to send a udp gso packet without checksum offload into the gso stack by disabling tx checksum (UDP_NO_CHECK6_TX).

Impressive...

Quoted 12 lines

This triggered the skb_warn_bad_offload.

  RIP: 0010:skb_warn_bad_offload+0x2bc/0x600 net/core/dev.c:2658
   skb_gso_segment include/linux/netdevice.h:4038 [inline]
   validate_xmit_skb+0x54d/0xd90 net/core/dev.c:3120
   __dev_queue_xmit+0xbf8/0x34c0 net/core/dev.c:3577
   dev_queue_xmit+0x17/0x20 net/core/dev.c:3618

UDP_NO_CHECK6_TX sets skb->ip_summed to CHECKSUM_NONE just after the udp gso integrity checks in udp_(v6_)send_skb. Extend those checks to catch and fail in this case.

Quoted 5 lines

After the integrity checks jump directly to the CHECKSUM_PARTIAL case to avoid reading the no_check_tx flags again (a TOCTTOU race).

Fixes bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
Signed-off-by Willem de Bruijn <[email protected]>

Applied, thanks Willem.

#2 lore raw
In reply to Willem de Bruijn
Quoted 12 lines · Willem de Bruijn

From: Willem de Bruijn <[email protected]>

Syzbot managed to send a udp gso packet without checksum offload into the gso stack by disabling tx checksum (UDP_NO_CHECK6_TX). This triggered the skb_warn_bad_offload.

  RIP: 0010:skb_warn_bad_offload+0x2bc/0x600 net/core/dev.c:2658
   skb_gso_segment include/linux/netdevice.h:4038 [inline]
   validate_xmit_skb+0x54d/0xd90 net/core/dev.c:3120
   __dev_queue_xmit+0xbf8/0x34c0 net/core/dev.c:3577
   dev_queue_xmit+0x17/0x20 net/core/dev.c:3618
Quoted 3 lines

UDP_NO_CHECK6_TX sets skb->ip_summed to CHECKSUM_NONE just after the udp gso integrity checks in udp_(v6_)send_skb. Extend those checks to catch and fail in this case.

Sounds rather familiar... perhaps we might want to check other exceptions added to UFO over the years, some might apply here as well.

Michal Kubecek

author maintainer
#3 lore raw
Quoted 13 lines · Michal Kubecek
Quoted 12 lines · Willem de Bruijn

From: Willem de Bruijn <[email protected]>

Syzbot managed to send a udp gso packet without checksum offload into the gso stack by disabling tx checksum (UDP_NO_CHECK6_TX). This triggered the skb_warn_bad_offload.

  RIP: 0010:skb_warn_bad_offload+0x2bc/0x600 net/core/dev.c:2658
   skb_gso_segment include/linux/netdevice.h:4038 [inline]
   validate_xmit_skb+0x54d/0xd90 net/core/dev.c:3120
   __dev_queue_xmit+0xbf8/0x34c0 net/core/dev.c:3577
   dev_queue_xmit+0x17/0x20 net/core/dev.c:3618
Quoted 6 lines
Quoted 3 lines

UDP_NO_CHECK6_TX sets skb->ip_summed to CHECKSUM_NONE just after the udp gso integrity checks in udp_(v6_)send_skb. Extend those checks to catch and fail in this case.

Sounds rather familiar... perhaps we might want to check other exceptions added to UFO over the years, some might apply here as well.

Good point. Of the conditions in that infamous branch

        if ((((length + (skb ? skb->len : fragheaderlen)) > mtu) ||
             (skb && skb_is_gso(skb))) &&
            (sk->sk_protocol == IPPROTO_UDP) &&
            (rt->dst.dev->features & NETIF_F_UFO) && !dst_xfrm(&rt->dst) &&
            (sk->sk_type == SOCK_DGRAM) && !sk->sk_no_check_tx) {

the fragmentation related ones do not apply. The new condition is also simpler as it is evaluated once when the entire datagram is built, as opposed to on each append.

That leaves the xfrm test. Indeed, those need to be blocked as well.