netdev - Re: [PATCH net] tcp: fix receive window update in tcp_add

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: <CACSApvYuLnfMupbn3yBK=0yHC-MXx-20-3D4oy_gqJW6Ynkn=A@mail.gmail.com>
Date:   Mon, 5 Oct 2020 09:50:34 -0400
From:   Soheil Hassas Yeganeh <soheil@...gle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Neal Cardwell <ncardwell@...gle.com>,
        Yuchung Cheng <ycheng@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Alexandre Ferrieux <alexandre.ferrieux@...nge.com>
Subject: Re: [PATCH net] tcp: fix receive window update in tcp_add_backlog()

On Mon, Oct 5, 2020 at 9:48 AM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
>
> We got reports from GKE customers flows being reset by netfilter
> conntrack unless nf_conntrack_tcp_be_liberal is set to 1.
>
> Traces seemed to suggest ACK packet being dropped by the
> packet capture, or more likely that ACK were received in the
> wrong order.
>
>  wscale=7, SYN and SYNACK not shown here.
>
>  This ACK allows the sender to send 1871*128 bytes from seq 51359321 :
>  New right edge of the window -> 51359321+1871*128=51598809
>
>  09:17:23.389210 IP A > B: Flags [.], ack 51359321, win 1871, options [nop,nop,TS val 10 ecr 999], length 0
>
>  09:17:23.389212 IP B > A: Flags [.], seq 51422681:51424089, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 1408
>  09:17:23.389214 IP A > B: Flags [.], ack 51422681, win 1376, options [nop,nop,TS val 10 ecr 999], length 0
>  09:17:23.389253 IP B > A: Flags [.], seq 51424089:51488857, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 64768
>  09:17:23.389272 IP A > B: Flags [.], ack 51488857, win 859, options [nop,nop,TS val 10 ecr 999], length 0
>  09:17:23.389275 IP B > A: Flags [.], seq 51488857:51521241, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 32384
>
>  Receiver now allows to send 606*128=77568 from seq 51521241 :
>  New right edge of the window -> 51521241+606*128=51598809
>
>  09:17:23.389296 IP A > B: Flags [.], ack 51521241, win 606, options [nop,nop,TS val 10 ecr 999], length 0
>
>  09:17:23.389308 IP B > A: Flags [.], seq 51521241:51553625, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 32384
>
>  It seems the sender exceeds RWIN allowance, since 51611353 > 51598809
>
>  09:17:23.389346 IP B > A: Flags [.], seq 51553625:51611353, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 57728
>  09:17:23.389356 IP B > A: Flags [.], seq 51611353:51618393, ack 1577, win 268, options [nop,nop,TS val 999 ecr 10], length 7040
>
>  09:17:23.389367 IP A > B: Flags [.], ack 51611353, win 0, options [nop,nop,TS val 10 ecr 999], length 0
>
>  netfilter conntrack is not happy and sends RST
>
>  09:17:23.389389 IP A > B: Flags [R], seq 92176528, win 0, length 0
>  09:17:23.389488 IP B > A: Flags [R], seq 174478967, win 0, length 0
>
>  Now imagine ACK were delivered out of order and tcp_add_backlog() sets window based on wrong packet.
>  New right edge of the window -> 51521241+859*128=51631193
>
> Normally TCP stack handles OOO packets just fine, but it
> turns out tcp_add_backlog() does not. It can update the window
> field of the aggregated packet even if the ACK sequence
> of the last received packet is too old.
>
> Many thanks to Alexandre Ferrieux for independently reporting the issue
> and suggesting a fix.
>
> Fixes: 4f693b55c3d2 ("tcp: implement coalescing on backlog queue")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: Alexandre Ferrieux <alexandre.ferrieux@...nge.com>

Acked-by: Soheil Hassas Yeganeh <soheil@...gle.com>

Thanks for the fix!

> ---
>  net/ipv4/tcp_ipv4.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 5084333b5ab647ca8ed296235a1ed6573693b250..592c7396272315c864372c158a7bc8850c6ddc61 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1788,12 +1788,12 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
>
>         __skb_pull(skb, hdrlen);
>         if (skb_try_coalesce(tail, skb, &fragstolen, &delta)) {
> -               thtail->window = th->window;
> -
>                 TCP_SKB_CB(tail)->end_seq = TCP_SKB_CB(skb)->end_seq;
>
> -               if (after(TCP_SKB_CB(skb)->ack_seq, TCP_SKB_CB(tail)->ack_seq))
> +               if (likely(!before(TCP_SKB_CB(skb)->ack_seq, TCP_SKB_CB(tail)->ack_seq))) {
>                         TCP_SKB_CB(tail)->ack_seq = TCP_SKB_CB(skb)->ack_seq;
> +                       thtail->window = th->window;
> +               }
>
>                 /* We have to update both TCP_SKB_CB(tail)->tcp_flags and
>                  * thtail->fin, so that the fast path in tcp_rcv_established()
> --
> 2.28.0.806.g8561365e88-goog
>