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

[Jakub Kicinski <kuba@kernel.org>]

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

> 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?

> > This has been caught by net/tap and net/psock_send.sh tests.  
> 
> That's very nice!
> 
> > Fixes: e269d79c7d35 ("net: missing check virtio")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>  
> 
> > +               if (csum_needed) {
> > +                       unsigned int p_rem, p_size;
> > +
> > +                       p_size = gso_size;
> > +                       p_rem = (skb->len - nh_off) % gso_size;
> > +                       if (p_rem)
> > +                               p_size = p_rem;
> > +
> > +                       /* Make sure csum still within packet after GSO */
> > +                       if (p_size + nh_off < csum_needed)
> > +                               return -EINVAL;
> > +               }
> > +  
> 
> A check could even go in the below branch.
> 
> The warning apparently was not that csum_needed is outside the segment
> entirely, but that the segment is non-linear and csum_start points in
> the non-linear part (offset >= skb_headlen(skb)).

Yes, I don't think the fix actually fixed the quoted warning :/
I decided to redo what it seem to have intended to fix in an un-broken
way, but the underlying issue is different.

> I don't think we should be playing SKBFL_SHARED_FRAG tricks to trigger
> linearization, to be clear.
> 
> We also cannot just silence the WARN and trust that the stack detects
> these bad packets and drops them (as ip_do_fragment does), as they
> might end up not in ip_do_fragment, but in a device ndo_start_xmit.
> 
> >                 /* Too small packets are not really GSO ones. */
> >                 if (skb->len - nh_off > gso_size) {
> >                         shinfo->gso_size = gso_size;