netdev - Re: [PATCH net] virtio: fix GSO with frames unaligned to size

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] [day] [month] [year] [list]

Message-ID: <CAF=yD-J7P=5V+Ksx9KRT2nm+UzK42HoBVCDed-EYH4KzMPWtkw@mail.gmail.com>
Date: Thu, 25 Jul 2024 17:02:15 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Denis Arefev <arefev@...mel.ru>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com, 
	eperezma@...hat.com, jasowang@...hat.com, linux-kselftest@...r.kernel.org, 
	mst@...hat.com, netdev@...r.kernel.org, pabeni@...hat.com, shuah@...nel.org, 
	virtualization@...ts.linux.dev, xuanzhuo@...ux.alibaba.com
Subject: Re: [PATCH net] virtio: fix GSO with frames unaligned to size

On Thu, Jul 25, 2024 at 10:27 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Thu, Jul 25, 2024 at 5:22 AM Denis Arefev <arefev@...mel.ru> wrote:
> >
> > I checked the patch on three reproducers and all three DEFINITELY broke the core.
> >
> > There are two malfunctions.
> >
> > 1. No flag skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG;
> >  If it is not set then __skb_linearize will not be executed in skb_checksum_help.
> >  sk_buff remains fragmented (non-linear) and this is the first warning.
> >  OR add skb_shinfo(skb)->tx_flags |= SKBFL_SHARED_FRAG.
> >  OR ask Eric Dumazet (cef401de7be8c). Is checking if (skb_has_shared_frag(skb)) so important?
> >  in the skb_checksum_help function, is it enough if (skb_is_nonlinear(skb)) ?
>
> Thanks for sharing the reproducers. Having a look.

Reproduced https://syzkaller.appspot.com/bug?extid=e1db31216c789f552871

That is against a v6.1 kernel, and the syzkaller page reports that it
did not fail against a recent upstream commit. Will take a closer look
at that.

But on v6.1, at least, the following did catch it:

@@ -72,6 +72,18 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
        if (thlen < sizeof(*th))
                goto out;

+       if (skb->ip_summed == CHECKSUM_PARTIAL &&
+           skb->csum_start != skb->transport_header) {
+               skb_dump(KERN_INFO, skb, false);
+               goto out;
+        }
+

And the geometry of the bad packet at that point:

[   52.003050][ T8403] skb len=12202 headroom=244 headlen=12093 tailroom=0
[   52.003050][ T8403] mac=(168,24) mac_len=24 net=(192,52) trans=244
[   52.003050][ T8403] shinfo(txflags=0 nr_frags=1 gso(size=1552 type=3 segs=0))
[   52.003050][ T8403] csum(0x60000c7 start=199 offset=1536
ip_summed=3 complete_sw=0 valid=0 level=0)

Sharing sketch patch for any feedback. A few downsides:

The patch adds a branch in the semi hot path of TCP software
segmentation for every packet. Including for the more common kernel
stack generated packets. And it needs the same test in two locations
in net/ipv4/udp_offload.c, for USO and UFO.

It is tempting to move it to the if (skb_gso_ok(skb, features |
NETIF_F_GSO_ROBUST)) branch below, as then it is limited to
SKB_GSO_DODGY. But that does not catch dodgy packets that need
software segmentation. Conversely, we could check in skb_segment
before calling skb_checksum_help.

I'll be out for four days over the weekend. May have to delay until next week.

> > Should we revert that and create a new fix against the original issue?
>
> We can, no strong preference.

On second thought, since this has to go to all the stable trees, let's
keep it a single patch. Rather than a revert + new fix.