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@xxxxxxxxx/T/#u > > 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.