Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220319110422.8261-1-zhouzhouyi@gmail.com> <CANn89iK46rw910CUJV3Kgf=M=HA32_ctd0xragwcRnHCV_VhmQ@mail.gmail.com>
 <CAABZP2yK2vCJcReJ_VvcqbkuEekvBpBJCyZ2geG=f83fv_RC=Q@mail.gmail.com> <CADVnQy=shHKbvf4OZjX5-3CnFPOm3zyexbaH9XTLZBMk6pxeew@mail.gmail.com>
In-Reply-To: <CADVnQy=shHKbvf4OZjX5-3CnFPOm3zyexbaH9XTLZBMk6pxeew@mail.gmail.com>
From:   Zhouyi Zhou <zhouzhouyi@gmail.com>
Date:   Sat, 19 Mar 2022 23:31:30 +0800
Message-ID: <CAABZP2we5YyL=Z0rk7vVry76OjgQ+YaMu1y8xCU6Cf7VnJ9JCg@mail.gmail.com>
Subject: Re: [PATCH v2] net:ipv4: send an ack when seg.ack > snd.nxt
To:     Neal Cardwell <ncardwell@google.com>
Cc:     Eric Dumazet <edumazet@google.com>,
        Florian Westphal <fw@strlen.de>,
        David Miller <davem@davemloft.net>, yoshfuji@linux-ipv6.org,
        dsahern@kernel.org, Jakub Kicinski <kuba@kernel.org>,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Wei Xu <xuweihf@ustc.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thank Neil and Eric for your valuable advice!

I will do the test and analysis. Please forgive my hasty reply because
it will take me some time to fully understand the email.  Also please
give me about a month to accomplish the test and analysis.
On Sat, Mar 19, 2022 at 9:57 PM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Sat, Mar 19, 2022 at 7:34 AM Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> >
> > Thanks for reviewing my patch
> >
> > On Sat, Mar 19, 2022 at 7:14 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Sat, Mar 19, 2022 at 4:04 AM <zhouzhouyi@gmail.com> wrote:
> > > >
> > > > From: Zhouyi Zhou <zhouzhouyi@gmail.com>
> > > >
> > > > In RFC 793, page 72: "If the ACK acks something not yet sent
> > > > (SEG.ACK > SND.NXT) then send an ACK, drop the segment,
> > > > and return."
> > > >
> > > > Fix Linux's behavior according to RFC 793.
> > > >
> > > > Reported-by: Wei Xu <xuweihf@ustc.edu.cn>
> > > > Signed-off-by: Wei Xu <xuweihf@ustc.edu.cn>
> > > > Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> > > > ---
> > > > Thank Florian Westphal for pointing out
> > > > the potential duplicated ack bug in patch version 1.
> > >
> > > I am travelling this week, but I think your patch is not necessary and
> > > might actually be bad.
> > >
> > > Please provide more details of why nobody complained of this until today.
> > >
> > > Also I doubt you actually fully tested this patch, sending a V2 30
> > > minutes after V1.
> > >
> > > If yes, please provide a packetdrill test.
> > I am a beginner to TCP, although I have submitted once a patch to
> > netdev in 2013 (aaa0c23cb90141309f5076ba5e3bfbd39544b985), this is
> > first time I learned packetdrill test.
> > I think I should do the packetdrill test in the coming days, and
> > provide more details of how this (RFC793 related) can happen.
>
> In addition to a packetdrill test and a more detailed analysis of how
> this can happen, and the implications, I think there are at least a
> few other issues that need to be considered:
>
> (1) AFAICT, adding an unconditional ACK if (after(ack, tp->snd_nxt))
> seems to open the potential for attackers to cause DoS attacks with
> something like the following:
>
>  (a) attacker injects one data packet in the A->B direction and one
> data packet in the B->A direction
>
>  (b) endpoint A sends an ACK for the forged data sent to it, which
> will have an ACK beyond B's snd_nxt
>
>  (c) endpoint B sends an ACK for the forged data sent to it, which
> will have an ACK beyond A's snd_nxt
>
>  (d) endpoint B receives the ACK sent by A, causing B to send another
> ACK beyond A's snd_nxt
>
>  (e) endpoint A receives the ACK sent by B, causing A to send another
> ACK beyond B's snd_nxt
>
>  (f) repeat (d) and (e) ad infinitum
I will make a full understanding of the above scenery in the coming days.
>
> So AFAICT an attacker could send two data packets with 1 byte of data
> and cause the two endpoints to use up an unbounded amount of CPU and
> bandwidth sending ACKs in an "infinite loop".
>
> To avoid this "infinite loop" of packets, if we really need to add an
> ACK in this case then the code should use the tcp_oow_rate_limited()
> helper to ensure that such ACKs are rate-limited. For more context on
> tcp_oow_rate_limited(), see:
>
> f06535c599354 Merge branch 'tcp_ack_loops'
> 4fb17a6091674 tcp: mitigate ACK loops for connections as tcp_timewait_sock
> f2b2c582e8242 tcp: mitigate ACK loops for connections as tcp_sock
> a9b2c06dbef48 tcp: mitigate ACK loops for connections as tcp_request_sock
> 032ee4236954e tcp: helpers to mitigate ACK loops by rate-limiting
> out-of-window dupacks
>
> Note that f06535c599354 in particular mentions the case discussed in this patch:
>
>     (2) RFC 793 (section 3.9, page 72) says: "If the ACK acknowledges
>         something not yet sent (SEG.ACK > SND.NXT) then send an ACK".
>
> (2) Please consider the potential that adding a new ACK in this
> scenario may introduce new, unanticipated side channels. For more on
> side channels, see:
>
>   https://lwn.net/Articles/696868/
>   The TCP "challenge ACK" side channel
I will read the article in the days following.
>
>   Principled Unearthing of TCP Side Channel Vulnerabilities
>   https://dl.acm.org/doi/10.1145/3319535.3354250
I will read the paper too.
>
> best regards,
> neal
Best Regards
Zhouyi