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]

Date: Thu, 12 Oct 2023 08:29:17 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Wang <jasowang@...hat.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 Thu, Oct 12, 2023 at 4:00 AM Jason Wang <jasowang@...hat.com> wrote:
>
> 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.

Implicitly it is part of the ABI, but so are other sensible
limitations, such as MAX_SKB_FRAGS. The limit was chosen high enough
to be unlikely to be a barrier to normal segmentation operations.