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
| ||
|
Message-ID: <CACGkMEu+eMhNhkY0Aw-ahD_4pGKbDD58aP=KhYV_9KT3odN-Sg@mail.gmail.com> 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