Messages in this thread | | | From | Neal Cardwell <> | Date | Sat, 19 Mar 2022 09:57:28 -0400 | Subject | Re: [PATCH v2] net:ipv4: send an ack when seg.ack > snd.nxt |
| |
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
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
Principled Unearthing of TCP Side Channel Vulnerabilities https://dl.acm.org/doi/10.1145/3319535.3354250
best regards, neal
| |