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