netdev - Re: [PATCH net] tcp: gso: really support BIG TCP

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]

Date: Fri, 02 Jun 2023 07:24:19 -0700
From: Alexander H Duyck <alexander.duyck@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, Alexander Duyck
	 <alexanderduyck@...a.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>,  Paolo Abeni <pabeni@...hat.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Xin Long
 <lucien.xin@...il.com>, David Ahern <dsahern@...nel.org>, 
 "eric.dumazet@...il.com" <eric.dumazet@...il.com>
Subject: Re: [PATCH net] tcp: gso: really support BIG TCP

On Fri, 2023-06-02 at 04:30 +0200, Eric Dumazet wrote:
> On Thu, Jun 1, 2023 at 11:46 PM Alexander Duyck <alexanderduyck@...a.com> wrote:
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Eric Dumazet <edumazet@...gle.com>
> > > Sent: Thursday, June 1, 2023 2:18 PM
> > > To: David S . Miller <davem@...emloft.net>; Jakub Kicinski
> > > <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>
> > > Cc: netdev@...r.kernel.org; Xin Long <lucien.xin@...il.com>; David Ahern
> > > <dsahern@...nel.org>; eric.dumazet@...il.com; Eric Dumazet
> > > <edumazet@...gle.com>; Alexander Duyck <alexanderduyck@...a.com>
> > > Subject: [PATCH net] tcp: gso: really support BIG TCP
> > > 
> > > > 
> > > We missed that tcp_gso_segment() was assuming skb->len was smaller than
> > > 65535 :
> > > 
> > > oldlen = (u16)~skb->len;
> > > 
> > > This part came with commit 0718bcc09b35 ("[NET]: Fix CHECKSUM_HW GSO
> > > problems.")
> > > 
> > > This leads to wrong TCP checksum.
> > > 
> > > Simply use csum_fold() to support 32bit packet lengthes.
> > > 
> > > oldlen name is a bit misleading, as it is the contribution of skb->len on the
> > > input skb TCP checksum. I added a comment to clarify this point.
> > > 
> > > Fixes: 09f3d1a3a52c ("ipv6/gso: remove temporary HBH/jumbo header")
> > > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > > Cc: Alexander Duyck <alexanderduyck@...com>
> > > ---
> > >  net/ipv4/tcp_offload.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index
> > > 45dda788938704c3f762256266d9ea29b6ded4a5..5a1a163b2d859696df8f204b5
> > > 0e3fc76c14b64e9 100644
> > > --- a/net/ipv4/tcp_offload.c
> > > +++ b/net/ipv4/tcp_offload.c
> > > @@ -75,7 +75,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> > >       if (!pskb_may_pull(skb, thlen))
> > >               goto out;
> > > 
> > > -     oldlen = (u16)~skb->len;
> > > +     /* Contribution of skb->len in current TCP checksum */
> > > +     oldlen = (__force u32)csum_fold((__force __wsum)skb->len);
> > >       __skb_pull(skb, thlen);
> > > 
> > >       mss = skb_shinfo(skb)->gso_size;
> > 
> > The only thing I am not sure about with this change is if we risk overflowing a u32 with all the math that may occur. The original code couldn't exceed a u16 for delta since we were essentially adding -oldlen + new header + mss. With us now allowing the use of a value larger than 16 bit we should be able to have the resultant value exceed 16b which means we might overflow when we add it to the current checksum.
> > 
> > As such we may want to look at holding off on the csum_fold until after we have added the new header and mss and then store the folded value in delta.
> > 
> 
> I think you missed that csum_fold() result is also a 16bit value.

I saw that. My concern was more about delta versus the oldlen value
itself though. Specifically your folded value is added to thlen + mss
which can then overflow past a 16b value, and when byteswapped and then
added to the original checksum there is a risk of potential overflow.

The general idea was that ~skb->len + (segment length) will always
technically be less than 0 since the original skb->len should always be
larger or equal to the new segmented length. So the code as it was
would always generate a value 16 or less in length.

This was important when we computed delta and added it to the original
value since we were using htonl which would byteswap things so we could
potentially generate a 32b value, but it couldn't overflow since the
two addends consisted of the upper 16b and lower 16b.

That is why I am thinking we are better off just dropping the "(u16)"
cast and just passing ~skb->len as the old_len.

To address this we then have a couple different approaches we could
take:
1. use csum_fold on the "delta" value either before or after the htonl.
2. use csum_add instead "+" for the addition of (th->check + delta)

I'm thinking option 2 may be the better way to go as it would just add
2 addc operations, one for newcheck and one at the end of the function
for th->check.