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