[PATCH net-next] udp: disable gso with no_check_tx

[PATCH net-next] udp: disable gso with no_check_tx

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

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 <willemb@google.com>
---
 net/ipv4/udp.c | 4 ++++
 net/ipv6/udp.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 794aeafeb782..dd3102a37ef9 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -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;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 6acfdd3e442b..a34e28ac03a7 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -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
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH net-next] udp: disable gso with no_check_tx

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 30 Apr 2018 15:58:36 -0400

> From: Willem de Bruijn <willemb@google.com>
> 
> Syzbot managed to send a udp gso packet without checksum offload into
> the gso stack by disabling tx checksum (UDP_NO_CHECK6_TX).

Impressive...

> 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 <willemb@google.com>

Applied, thanks Willem.

Re: [PATCH net-next] udp: disable gso with no_check_tx

[Michal Kubecek]

On Mon, Apr 30, 2018 at 03:58:36PM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> 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.

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

Re: [PATCH net-next] udp: disable gso with no_check_tx

[Willem de Bruijn]

On Wed, May 2, 2018 at 9:05 AM, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Mon, Apr 30, 2018 at 03:58:36PM -0400, Willem de Bruijn wrote:
>> From: Willem de Bruijn <willemb@google.com>
>>
>> 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.
>
> 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.