netdev - Re: [PATCH net v2] net: drop bad gso csum_start and offset in virtio_net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <66df9d10e88d2_81fd329432@willemb.c.googlers.com.notmuch>
Date: Mon, 09 Sep 2024 21:12:48 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Jason Wang <jasowang@...hat.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>, 
 Szabolcs Nagy <szabolcs.nagy@....com>, 
 netdev@...r.kernel.org, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 arefev@...mel.ru, 
 alexander.duyck@...il.com, 
 Willem de Bruijn <willemb@...gle.com>, 
 stable@...r.kernel.org, 
 Jakub Sitnicki <jakub@...udflare.com>, 
 Felix Fietkau <nbd@....name>, 
 Mark Brown <broonie@...nel.org>, 
 Yury Khrustalev <yury.khrustalev@....com>, 
 nd@....com
Subject: Re: [PATCH net v2] net: drop bad gso csum_start and offset in
 virtio_net_hdr

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@gmail.com/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.