netdev - Re: [PATCH net] gso: fix gso fraglist segmentation after pull from frag

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68d4c9cf-f61f-4257-8d98-9a363142cc53@nbd.name>
Date: Wed, 25 Sep 2024 22:59:55 +0200
From: Felix Fietkau <nbd@....name>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
 pabeni@...hat.com, stable@...r.kernel.org, maze@...gle.com,
 shiming.cheng@...iatek.com, daniel@...earbox.net, lena.wang@...iatek.com,
 herbert@...dor.apana.org.au, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] gso: fix gso fraglist segmentation after pull from
 frag_list

On 25.09.24 21:09, Willem de Bruijn wrote:
> Felix Fietkau wrote:
>> On 22.09.24 17:03, Willem de Bruijn wrote:
>> > From: Willem de Bruijn <willemb@...gle.com>
>> > 
>> > Detect gso fraglist skbs with corrupted geometry (see below) and
>> > pass these to skb_segment instead of skb_segment_list, as the first
>> > can segment them correctly.
>> > 
>> > Valid SKB_GSO_FRAGLIST skbs
>> > - consist of two or more segments
>> > - the head_skb holds the protocol headers plus first gso_size
>> > - one or more frag_list skbs hold exactly one segment
>> > - all but the last must be gso_size
>> > 
>> > Optional datapath hooks such as NAT and BPF (bpf_skb_pull_data) can
>> > modify these skbs, breaking these invariants.
>> > 
>> > In extreme cases they pull all data into skb linear. For UDP, this
>> > causes a NULL ptr deref in __udpv4_gso_segment_list_csum at
>> > udp_hdr(seg->next)->dest.
>> > 
>> > Detect invalid geometry due to pull, by checking head_skb size.
>> > Don't just drop, as this may blackhole a destination. Convert to be
>> > able to pass to regular skb_segment.
>> > 
>> > Link: https://lore.kernel.org/netdev/20240428142913.18666-1-shiming.cheng@mediatek.com/
>> > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
>> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
>> > Cc: stable@...r.kernel.org
>> > 
>> > ---
>> > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> > index d842303587af..e457fa9143a6 100644
>> > --- a/net/ipv4/udp_offload.c
>> > +++ b/net/ipv4/udp_offload.c
>> > @@ -296,8 +296,16 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>> >   		return NULL;
>> >   	}
>> >   
>> > -	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST)
>> > -		return __udp_gso_segment_list(gso_skb, features, is_ipv6);
>> > +	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST) {
>> > +		 /* Detect modified geometry and pass these to skb_segment. */
>> > +		if (skb_pagelen(gso_skb) - sizeof(*uh) == skb_shinfo(gso_skb)->gso_size)
>> > +			return __udp_gso_segment_list(gso_skb, features, is_ipv6);
>> > +
>> > +		 /* Setup csum, as fraglist skips this in udp4_gro_receive. */
>> > +		gso_skb->csum_start = skb_transport_header(gso_skb) - gso_skb->head;
>> > +		gso_skb->csum_offset = offsetof(struct udphdr, check);
>> > +		gso_skb->ip_summed = CHECKSUM_PARTIAL;
>> 
>> I also noticed this uh->check update done by udp4_gro_complete only in 
>> case of non-fraglist GRO:
>> 
>>      if (uh->check)
>>          uh->check = ~udp_v4_check(skb->len - nhoff, iph->saddr,
>>                        iph->daddr, 0);
>> 
>> I didn't see any equivalent in your patch. Is it missing or left out 
>> intentionally?
> 
> Thanks. That was not intentional. I think you're right. Am a bit
> concerned that all this testing did not catch it. Perhaps because
> CHECKSUM_PARTIAL looped to ingress on the same machine is simply
> interpreted as CHECKSUM_UNNECESSARY. Need to look into that.
> 
> If respinning this, I should also change the Fixes to
> 
> Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> 
> Analogous to the eventual TCP fix to
> 
> Fixes: bee88cd5bd83 ("net: add support for segmenting TCP fraglist GSO packets")

In the mean time, I've been working on the TCP side. I managed to
reproduce the issue on one of my devices by routing traffic from
Ethernet to Wifi using your BPF test program.

The following patch makes it work for me for TCP v4. Still need to
test and fix v6.

- Felix

---
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -101,8 +101,20 @@ static struct sk_buff *tcp4_gso_segment(struct sk_buff *skb,
  	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
  		return ERR_PTR(-EINVAL);
  
-	if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST)
-		return __tcp4_gso_segment_list(skb, features);
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) {
+		struct tcphdr *th = tcp_hdr(skb);
+		const struct iphdr *iph;
+
+		if (skb_pagelen(skb) - th->doff * 4 == skb_shinfo(skb)->gso_size)
+			return __tcp4_gso_segment_list(skb, features);
+
+		iph = ip_hdr(skb);
+		skb_shinfo(skb)->gso_type &= ~SKB_GSO_FRAGLIST;
+		skb->csum_start = (unsigned char *)th - skb->head;
+		skb->csum_offset = offsetof(struct tcphdr, check);
+		skb->ip_summed = CHECKSUM_PARTIAL;
+		th->check = ~tcp_v4_check(skb->len, iph->saddr, iph->daddr, 0);
+	}
  
  	if (unlikely(skb->ip_summed != CHECKSUM_PARTIAL)) {
  		const struct iphdr *iph = ip_hdr(skb);