Date: Mon, 16 Oct 2023 14:39:42 +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 Fri, Oct 13, 2023 at 7:40 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Thu, Oct 12, 2023 at 9:30 PM Jason Wang <jasowang@...hat.com> wrote:
> >
> > 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.
>
> Perhaps MAX_SKB_FRAGS is not the best example. But there are other
> conditions that are discoverable by validation returning an error when
> outside the bounds of normal operation.
>
> UDP_MAX_SEGMENTS is also not explicitly exposed to UDP_SEGMENT socket
> users, without issues.
>
> If absolutely needed, the boundary can be detected through probing.

See above, probing can only be done during driver probe.

> But it should not be needed as chosen to be well outside normal
> operating range.
>
> A secondary benefit is that future kernels can relax (but not tighten)
> the restriction if needed. The current limit was chosen with the usual
> 64KB / 1500B operating default in mind. If we would extend BIGTCP to
> UDP, the existing limit of 64 might need relaxing (for both virtio and
> sockets simultaneously). Anything ABI is set in stone, best to avoid
> if not strictly necessary.

The main concern is the migration, if we migrate from a Linux
hypervisor to another. Guests notice the difference in the limitation.

Thanks

>

Powered by blists - more mailing lists