| 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
| ||
|
Message-ID: <20240704195049.28475-1-kuniyu@amazon.com>
Date: Thu, 4 Jul 2024 12:50:49 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <edumazet@...gle.com>
CC: <brakmo@...com>, <davem@...emloft.net>, <dsahern@...nel.org>,
<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v1 net] tcp: Don't drop SYN+ACK for simultaneous connect().
From: Eric Dumazet <edumazet@...gle.com>
Date: Thu, 4 Jul 2024 21:01:52 +0200
> On Thu, Jul 4, 2024 at 7:36 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > From: Eric Dumazet <edumazet@...gle.com>
> > Date: Thu, 4 Jul 2024 10:44:55 +0200
> > > On Thu, Jul 4, 2024 at 5:57 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > > >
> > > > RFC 9293 states that in the case of simultaneous connect(), the connection
> > > > gets established when SYN+ACK is received. [0]
> > > >
> > > > TCP Peer A TCP Peer B
> > > >
> > > > 1. CLOSED CLOSED
> > > > 2. SYN-SENT --> <SEQ=100><CTL=SYN> ...
> > > > 3. SYN-RECEIVED <-- <SEQ=300><CTL=SYN> <-- SYN-SENT
> > > > 4. ... <SEQ=100><CTL=SYN> --> SYN-RECEIVED
> > > > 5. SYN-RECEIVED --> <SEQ=100><ACK=301><CTL=SYN,ACK> ...
> > > > 6. ESTABLISHED <-- <SEQ=300><ACK=101><CTL=SYN,ACK> <-- SYN-RECEIVED
> > > > 7. ... <SEQ=100><ACK=301><CTL=SYN,ACK> --> ESTABLISHED
> > > >
> > > > However, since commit 0c24604b68fc ("tcp: implement RFC 5961 4.2"), such a
> > > > SYN+ACK is dropped in tcp_validate_incoming() and responded with Challenge
> > > > ACK.
> > > >
> > > > For example, the write() syscall in the following packetdrill script fails
> > > > with -EAGAIN, and wrong SNMP stats get incremented.
> > > >
> > > > 0 socket(..., SOCK_STREAM|SOCK_NONBLOCK, IPPROTO_TCP) = 3
> > > > +0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
> > > >
> > > > +0 > S 0:0(0) <mss 1460,sackOK,TS val 1000 ecr 0,nop,wscale 8>
> > > > +0 < S 0:0(0) win 1000 <mss 1000>
> > > > +0 > S. 0:0(0) ack 1 <mss 1460,sackOK,TS val 3308134035 ecr 0,nop,wscale 8>
> > > > +0 < S. 0:0(0) ack 1 win 1000
> > > >
> > > > +0 write(3, ..., 100) = 100
> > > > +0 > P. 1:101(100) ack 1
> > > >
> > > > --
> > > >
> > > > # packetdrill cross-synack.pkt
> > > > cross-synack.pkt:13: runtime error in write call: Expected result 100 but got -1 with errno 11 (Resource temporarily unavailable)
> > > > # nstat
> > > > ...
> > > > TcpExtTCPChallengeACK 1 0.0
> > > > TcpExtTCPSYNChallenge 1 0.0
> > > >
> > > > That said, this is no big deal because the Challenge ACK finally let the
> > > > connection state transition to TCP_ESTABLISHED in both directions. If the
> > > > peer is not using Linux, there might be a small latency before ACK though.
> > >
> > > I suggest removing these 3 lines. Removing a not needed challenge ACK is good
> > > regardless of the 'other peer' behavior.
> >
> > I see, then should Fixes point to 0c24604b68fc ?
>
> I would target net-next, unless you have a very convincing reason.
>
> The bug might only be exposed by eBPF users, right ?
Yes, and I'm ok with net-next.
>
> >
> > Also I noticed it still sends ACK in tcp_ack_snd_check() as if it's a
> > response to the normal 3WHS, so we need:
> >
> > ---8<---
> > @@ -6788,6 +6793,9 @@ tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> > tcp_fast_path_on(tp);
> > if (sk->sk_shutdown & SEND_SHUTDOWN)
> > tcp_shutdown(sk, SEND_SHUTDOWN);
> > +
> > + if (!req)
> > + goto consume;
>
> I guess this is becoming a bit risky for net tree ?
>
> Given tcp cross syn is mostly used by fuzzers, I would advise doing
> something very minimal.
Is this still applied if I target to net-next ? I'm a bit confused
with that removing unnecessary ACK is good.
>
> > break;
> >
> > case TCP_FIN_WAIT1: {
> > ---8<---
> >
> > and I have a question regarding the consume: label. Why do we use
> > __kfree_skb() there instead of consume_skb() ? I guess it's because
> > skb_unref() is unnecessary and expensive and tracing is also expensive ?
>
> For the same reason we do __kfree_skb() in other places.
>
> This predates consume_skb().
That makes sense.
> > >
> > > >
> > > > The problem is that bpf_skops_established() is triggered by the Challenge
> > > > ACK instead of SYN+ACK. This causes the bpf prog to miss the chance to
> > > > check if the peer supports a TCP option that is expected to be exchanged
> > > > in SYN and SYN+ACK.
> > > >
> > > > Let's accept a bare SYN+ACK for non-TFO TCP_SYN_RECV sockets to avoid such
> > > > a situation.
> > > >
> > > > Link: https://www.rfc-editor.org/rfc/rfc9293.html#section-3.5-7 [0]
> > > > Fixes: 9872a4bde31b ("bpf: Add TCP connection BPF callbacks")
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > > ---
> > > > net/ipv4/tcp_input.c | 7 ++++++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > > index 77294fd5fd3e..70595009bb58 100644
> > > > --- a/net/ipv4/tcp_input.c
> > > > +++ b/net/ipv4/tcp_input.c
> > > > @@ -5980,6 +5980,11 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
> > > > * RFC 5961 4.2 : Send a challenge ack
> > > > */
> > > > if (th->syn) {
> > > > + if (sk->sk_state == TCP_SYN_RECV && !tp->syn_fastopen && th->ack &&
> > > > + TCP_SKB_CB(skb)->seq + 1 == TCP_SKB_CB(skb)->end_seq &&
> > > > + TCP_SKB_CB(skb)->seq + 1 == tp->rcv_nxt &&
> > > > + TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt)
> > > > + goto pass;
> > > > syn_challenge:
> > > > if (syn_inerr)
> > > > TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
> > > > @@ -5990,7 +5995,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
> > > > }
> > > >
> > > > bpf_skops_parse_hdr(sk, skb);
> > > > -
> > > > +pass:
> > >
> > > It is not clear to me why we do not call bpf_skops_parse_hdr(sk, skb)
> > > in this case ?
> >
> > I skipped bpf_skops_parse_hdr() as it had this check.
> >
> > switch (sk->sk_state) {
> > case TCP_SYN_RECV:
> > case TCP_SYN_SENT:
> > case TCP_LISTEN:
> > return;
> > }
>
> I think I prefer these checks being clearly centralized there, instead
> of trying to duplicate them earlier.
>
> This is slow path anyway.
Exactly, I'll move the label before bpf_skops_parse_hdr().
>
> I am a bit like Paolo : why do we even care, adding more fuel for fuzzers...
...for some BPF users wanting to cover all possible cases.. :S
Powered by blists - more mailing lists