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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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..







[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux