[PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
Signed-off-by: Willem de Bruijn <willemb@google.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;
+		}
 
 		/* Kernel has a special handling for GSO_BY_FRAGS. */
 		if (gso_size == GSO_BY_FRAGS)
-- 
2.42.0.609.gbb76f46606-goog

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Eric Dumazet]

On Wed, Oct 11, 2023 at 4:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> Signed-off-by: Willem de Bruijn <willemb@google.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@google.com>

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Jason Wang]

On Wed, Oct 11, 2023 at 10:01 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> Signed-off-by: Willem de Bruijn <willemb@google.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@redhat.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
>

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Willem de Bruijn]

On Thu, Oct 12, 2023 at 4:00 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 11, 2023 at 10:01 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> > Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> > Signed-off-by: Willem de Bruijn <willemb@google.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@redhat.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.

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Jason Wang]

On Thu, Oct 12, 2023 at 8:29 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Oct 12, 2023 at 4:00 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Oct 11, 2023 at 10:01 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> > > Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> > > Signed-off-by: Willem de Bruijn <willemb@google.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@redhat.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.
>

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Willem de Bruijn]

On Thu, Oct 12, 2023 at 9:30 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Oct 12, 2023 at 8:29 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Thu, Oct 12, 2023 at 4:00 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 10:01 PM Willem de Bruijn
> > > <willemdebruijn.kernel@gmail.com> wrote:
> > > >
> > > > From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> > > > Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> > > > Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> > > > Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> > > > Signed-off-by: Willem de Bruijn <willemb@google.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@redhat.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.
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.

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[Jason Wang]

On Fri, Oct 13, 2023 at 7:40 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Oct 12, 2023 at 9:30 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Oct 12, 2023 at 8:29 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > On Thu, Oct 12, 2023 at 4:00 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Wed, Oct 11, 2023 at 10:01 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@gmail.com> wrote:
> > > > >
> > > > > From: Willem de Bruijn <willemb@google.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@syzkaller.appspotmail.com
> > > > > Closes: https://lore.kernel.org/netdev/0000000000005039270605eb0b7f@google.com/
> > > > > Reported-by: syzbot+c99d835ff081ca30f986@syzkaller.appspotmail.com
> > > > > Closes: https://lore.kernel.org/netdev/0000000000005426680605eb0b9f@google.com/
> > > > > Signed-off-by: Willem de Bruijn <willemb@google.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@redhat.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

>

Re: [PATCH net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 11 Oct 2023 10:01:14 -0400 you wrote:
> From: Willem de Bruijn <willemb@google.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
> 
> [...]

Here is the summary with links:
  - [net] net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation
    https://git.kernel.org/netdev/net/c/fc8b2a619469

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html