netdev - Re: linux 5.17.1 disregarding ACK values resulting in stalled TCP connections

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:   Thu, 7 Apr 2022 14:48:33 +0200 (CEST)
From:   Jozsef Kadlecsik <kadlec@...filter.org>
To:     Florian Westphal <fw@...len.de>
cc:     Neal Cardwell <ncardwell@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Jaco Kroon <jaco@....co.za>, netfilter-devel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: linux 5.17.1 disregarding ACK values resulting in stalled TCP
 connections

On Thu, 7 Apr 2022, Florian Westphal wrote:

> Jozsef Kadlecsik <kadlec@...filter.org> wrote:
> > I'd merge the two conditions so that it'd cover both original condition 
> > branches:
> > 
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index 8ec55cd72572..87375ce2f995 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -556,33 +556,26 @@ static bool tcp_in_window(struct nf_conn *ct,
> >  			}
> >  
> >  		}
> > -	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
> > -		     && dir == IP_CT_DIR_ORIGINAL)
> > -		   || (state->state == TCP_CONNTRACK_SYN_RECV
> > -		     && dir == IP_CT_DIR_REPLY))
> > -		   && after(end, sender->td_end)) {
> > +	} else if (tcph->syn &&
> > +		   ((after(end, sender->td_end) &&
> > +		     (state->state == TCP_CONNTRACK_SYN_SENT ||
> > +		      state->state == TCP_CONNTRACK_SYN_RECV)) ||
> > +		    (dir == IP_CT_DIR_REPLY &&
> > +		     state->state == TCP_CONNTRACK_SYN_SENT))) {
> 
> Thats what I did as well, I merged the two branches but I made the
> 2nd clause stricter to also consider the after() test; it would no
> longer re-init for syn-acks when sequence did not advance.

That's perfectly fine.

But what about simultaneous syn? The TCP state is zeroed in the REPLY 
direction, so the after() test can easily be false and the state wouldn't 
be picked up. Therefore I extended the condition.

Best regards,
Jozsef
-
E-mail  : kadlec@...ckhole.kfki.hu, kadlecsik.jozsef@...ner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary