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: <9c6d2d7-70b-bd12-ee14-7923664afb1@netfilter.org>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ