Willem de Bruijn wrote:
Willem de Bruijn wrote:
So I guess VIRTIO_NET_HDR_GSO_* without VIRTIO_NET_HDR_F_DATA_VALID would be wrong on rx.
But the new check
if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) { [...] case SKB_GSO_TCPV4: case SKB_GSO_TCPV6: if (skb->csum_offset != offsetof(struct tcphdr, check)) return -EINVAL;should be limited to callers of virtio_net_hdr_to_skb on the tx/GSO path.
Looking what the cleanest/minimal patch is to accomplish that.
virtio_net_hdr_to_skb() translates virtio-net header to skb metadata, so it's RX. For TX the helper should be virtio_net_hdr_from_skb() which translates skb metadata to virtio hdr.
virtio_net_hdr_to_skb is used by PF_PACKET, tun and tap
Exactly.
when injecting a packet into the egress path.
For tuntap it's still the RX path. For PF_PACEKT and macvtap, it's the tx.
Maybe a new parameter to virtio_net_hdr_to_skb()?
This is the most straightforward approach. But requires changse to all callers, in a patch targeting all the stable branches.
I'd prefer if we can detect ingress vs egress directly.
Not doing this, because both on ingress and egress the allowed ip_summed types are more relaxed than I imagined.
Let's just make the check more narrow to avoid such false positives.
GRO indeed allows CHECKSUM_NONE.
But TSO also accepts packets that are not CHECKSUM_PARTIAL, and will fix up csum_start/csum_off. In tcp4_gso_segment:
if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) { const struct iphdr *iph = ip_hdr(skb); struct tcphdr *th = tcp_hdr(skb); /* Set up checksum pseudo header, usually expect stack to * have done this already. */ th->check = 0; skb->ip_summed = CHECKSUM_PARTIAL; __tcp_v4_send_check(skb, iph->saddr, iph->daddr); }With __tcp_v4_send_check:
void __tcp_v4_send_check(struct sk_buff *skb, __be32 saddr, __be32 daddr) { struct tcphdr *th = tcp_hdr(skb);
th->check = ~tcp_v4_check(skb->len, saddr, daddr, 0); skb->csum_start = skb_transport_header(skb) - skb->head; skb->csum_offset = offsetof(struct tcphdr, check);}
That means that we can relax the check on input from userspace to bad CHECKSUM_PARTIAL input:
@@ -173,7 +173,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, break; case SKB_GSO_TCPV4: case SKB_GSO_TCPV6:
if (skb->csum_offset != offsetof(struct tcphdr, check))
if (skb->ip_summed == CHECKSUM_PARTIAL &&skb->csum_offset != offsetof(struct tcphdr, check)) return -EINVAL;I've verified that this test still catches the bad packet from the syzkaller report in the Link in the commit.
Sent: https://lore.kernel.org/netdev/20240910004033.530313-1-willemdebruijn.kernel...
Based on ip_summed, pkt_type, is_skb_wmem or so. But so far have not found a suitable condition.
I noticed something else: as you point out TUN is ingress. Unlike virtnet_receive, it does not set ip_summed to CHECKSUM_UNNECESSARY if VIRTIO_NET_HDR_F_DATA_VALID. It probably should. GRO expects packets to have had their integrity verified. CHECKSUM_NONE on ingress is not correct for GRO.
Actually CHECKSUM_NONE is allowed. It just triggers software checksum validation.
Tun by default does not use GRO, only if enabling IFF_NAPI.
If a packet arrives at GRO with CHECKSUM_PARTIAL, then its checksum is assumed valid, per __skb_gro_checksum_validate_needed. So that would be one way for tun users today to get efficient GRO.
And also related: no GRO should be generated by a device unless VIRTIO_NET_HDR_F_DATA_VALID is also passed? I have to check the spec if it says anything about this.
Given that GRO handles !CHECKSUM_UNNECESSARY, probably no need to require VIRTIO_NET_HDR_F_DATA_VALID with virtio GRO either.