[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89iJ23Mz3tUXCdoLAHhNv0KKqRLG3k0J6aGVBio0bPyV-Bw@mail.gmail.com>
Date: Wed, 11 Oct 2023 16:10:39 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, andrew@...nix.com, jasowang@...hat.com,
Willem de Bruijn <willemb@...gle.com>, syzbot+01cdbc31e9c0ae9b33ac@...kaller.appspotmail.com,
syzbot+c99d835ff081ca30f986@...kaller.appspotmail.com
Subject: Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation
On Wed, Oct 11, 2023 at 4:01 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> From: Willem de Bruijn <willemb@...gle.com>
>
> Syzbot reported two new paths to hit an internal WARNING using the
> new virtio gso type VIRTIO_NET_HDR_GSO_UDP_L4.
>
> RIP: 0010:skb_checksum_help+0x4a2/0x600 net/core/dev.c:3260
> skb len=64521 gso_size=344
> and
>
> RIP: 0010:skb_warn_bad_offload+0x118/0x240 net/core/dev.c:3262
>
> Older virtio types have historically had loose restrictions, leading
> to many entirely impractical fuzzer generated packets causing
> problems deep in the kernel stack. Ideally, we would have had strict
> validation for all types from the start.
>
> New virtio types can have tighter validation. Limit UDP GSO packets
> inserted via virtio to the same limits imposed by the UDP_SEGMENT
> socket interface:
>
> 1. must use checksum offload
> 2. checksum offload matches UDP header
> 3. no more segments than UDP_MAX_SEGMENTS
> 4. UDP GSO does not take modifier flags, notably SKB_GSO_TCP_ECN
>
> Fixes: 860b7f27b8f7 ("linux/virtio_net.h: Support USO offload in vnet header.")
> Reported-by: syzbot+01cdbc31e9c0ae9b33ac@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> Reported-by: syzbot+c99d835ff081ca30f986@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
> include/linux/virtio_net.h | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 7b4dd69555e49..27cc1d4643219 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -3,8 +3,8 @@
> #define _LINUX_VIRTIO_NET_H
>
> #include <linux/if_vlan.h>
> +#include <linux/udp.h>
> #include <uapi/linux/tcp.h>
> -#include <uapi/linux/udp.h>
> #include <uapi/linux/virtio_net.h>
>
> static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
> @@ -151,9 +151,22 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> unsigned int nh_off = p_off;
> struct skb_shared_info *shinfo = skb_shinfo(skb);
>
> - /* UFO may not include transport header in gso_size. */
> - if (gso_type & SKB_GSO_UDP)
> + switch (gso_type & ~SKB_GSO_TCP_ECN) {
> + case SKB_GSO_UDP:
> + /* UFO may not include transport header in gso_size. */
> nh_off -= thlen;
> + break;
> + case SKB_GSO_UDP_L4:
> + if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> + return -EINVAL;
> + if (skb->csum_offset != offsetof(struct udphdr, check))
> + return -EINVAL;
> + if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> + return -EINVAL;
> + if (gso_type != SKB_GSO_UDP_L4)
> + return -EINVAL;
> + break;
> + }
>
Very nice, thanks Willem !
Reviewed-by: Eric Dumazet <edumazet@...gle.com>
Powered by blists - more mailing lists