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