Message-ID: <66a1bdf2955b5_85410294a0@willemb.c.googlers.com.notmuch>
Date: Wed, 24 Jul 2024 22:52:34 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, 
 netdev@...r.kernel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 mst@...hat.com, 
 jasowang@...hat.com, 
 xuanzhuo@...ux.alibaba.com, 
 eperezma@...hat.com, 
 shuah@...nel.org, 
 arefev@...mel.ru, 
 virtualization@...ts.linux.dev, 
 linux-kselftest@...r.kernel.org, 
 Alexander Duyck <alexander.duyck@...il.com>
Subject: Re: [PATCH net] virtio: fix GSO with frames unaligned to size

Jakub Kicinski wrote:
> On Tue, 23 Jul 2024 23:48:24 -0400 Willem de Bruijn wrote:
> > On Tue, Jul 23, 2024 at 3:31 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > > The commit under fixes added a questionable check to
> > > virtio_net_hdr_to_skb(). I'm guessing the check was supposed
> > > to protect from csum offset being outside of a segment
> > > (especially if len is not multiple of segment size).
> > >
> > > The condition can't be right, tho, as it breaks previously
> > > working sending of GSO frames with only one segment
> > > (i.e. when gso_size <= len we silently ignore the GSO
> > > request and send a single non-GSO frame).
> > >
> > > Fix the logic and move it to the GSO part.  
> > 
> > I missed the previous patch. Should we revert that and create a new
> > fix against the original issue?
> 
> We can, no strong preference.
> 
> > Normally the checksum start + offset should always be in the header,
> > so not even part of gso_size. So needed need not be related to
> > gso_size.
> > 
> > The exception to this is UDP fragmentation offload, I suppose. As
> > there the network and transport headers are part of the UFO payload.
> > 
> > But even for the normal TSO and USO cases we cannot verify in
> > virtio_net_hdr_to_skb that the csum_start + csum_off passed from
> > userspace are really pointing into the transport header.
> > 
> > For SKB_GSO_UDP_L4 I added a minimal check that csum_off must be
> > offsetof(struct udphdr, check). We can arguably tighten these csum_off
> > for all requests, as only UDP and TCP offsets are valid. But no such
> > simple check exists for csum_start. This requires full packet parsing,
> > which we don't do until skb_gso_segment.
> > 
> > One option may be to test csum_start in tcp_gso_segment and
> > udp_gso_fragment and fail segmentation when it points not where
> > expected.
> 
> That should work, I think.
> Should we still check the segment boundaries, tho?
> A bit worrying to have packets floating around the stack with clearly
> broken csum offset. At the same time maybe the modulo isn't free..

If we catch all cases during segmentation, then it's safe too.

Since these packets get SKB_GSO_DODGY, no risk of passing bad
packets anywhere else.

We also defer other correctness checks to segmentation already,
because else we end up building a second parsing stage here.

But overall I also prefer checking at the gate. So either way.
 
> > Btw, do we have a better idea what exact packet triggered this
> > WARN_ON_ONCE in skb_checksum_help? Usually, more interesting than the
> > skb_dump of the segment that reached the WARN is the skb_dump at the
> > time of virtio_net_hdr_to_skb, along with the vnet_hdr.
> 
> I don't have any extra info, beyond what's in the commit message :(
> Note that the syzbot report says 6.7, too.
> Denis, can you comment? Do you have a repro?

Yes, please share if there is a repro. The original report did credit
syzkaller.

Else I might have to look into building one..

Powered by blists - more mailing lists