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: <CACGkMEtTqJ9NWTE=V9QUh57b59Y7VzNU-4E2wjUpROpWy5nanw@mail.gmail.com>
Date: Fri, 13 Oct 2023 09:30:37 +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 Thu, Oct 12, 2023 at 8:29 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.

There's no easy to detect things like MAX_SKB_FRAGS or anything I miss
here? For example, guests can send a packet with s/g more than
MAX_SKB_FRAGS, TUN can arrange the skb allocation to make sure it
doesn't exceed the limitation. This is not the case for
UDP_MAX_SEGMENTS.

Thanks

> The limit was chosen high enough
> to be unlikely to be a barrier to normal segmentation operations.
>