netdev - Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGkMEuq3srKZWYsVQutfHOuJAyHDz4xCJWG3o6hs+W_HhZ2jQ@mail.gmail.com>
Date: Thu, 12 Oct 2023 16:00:29 +0800
From: Jason Wang <jasowang@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	edumazet@...gle.com, pabeni@...hat.com, andrew@...nix.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 10: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;

Acked-by: Jason Wang <jasowang@...hat.com>

But a question comes into my mind: whether the udp max segments should
be part of the virtio ABI or not.

Otherwise guests may notice subtle differences after migration?

Thanks


> +                       if (gso_type != SKB_GSO_UDP_L4)
> +                               return -EINVAL;
> +                       break;
> +               }
>
>                 /* Kernel has a special handling for GSO_BY_FRAGS. */
>                 if (gso_size == GSO_BY_FRAGS)
> --
> 2.42.0.609.gbb76f46606-goog
>