Willem de Bruijn wrote:

quoted

quoted

quoted

quoted

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.

quoted

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.

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.

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.