Re: [PATCH net-next] udp: disable gso with no_check_tx
On Wed, May 2, 2018 at 9:05 AM, Michal Kubecekwrote: > On Mon, Apr 30, 2018 at 03:58:36PM -0400, Willem de Bruijn wrote: >> From: Willem de Bruijn >> >> 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(>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.
Re: [PATCH net-next] udp: disable gso with no_check_tx
On Mon, Apr 30, 2018 at 03:58:36PM -0400, Willem de Bruijn wrote: > From: Willem de Bruijn> > 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
From: Willem de BruijnDate: Mon, 30 Apr 2018 15:58:36 -0400 > From: Willem de Bruijn > > 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 Applied, thanks Willem.
[PATCH net-next] udp: disable gso with no_check_tx
From: Willem de BruijnSyzbot 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 --- 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, >saddr, >daddr, len); goto send; } else -- 2.17.0.441.gb46fe60e1d-goog