Re: [PATCH] tcp: validate old ACKs before fast path data processing

For incoming TCP segments processed in the fast path,
Linux does not enforce the RFC5961 requirement:

   The ACK value is considered acceptable only if
   it is in the range of ((SND.UNA - MAX.SND.WND) <= SEG.ACK <=
   SND.NXT).  All incoming segments whose ACK value doesn't satisfy the
   above condition MUST be discarded and an ACK sent back.

Meaning the ack of incoming segments is no earlier than a window back
from the first unacknowledged sent byte.

Later work showed that the condition (SND.UNA - MAX.SND.WND) <= SEG.ACK
can be further tightened, eliminating some demonstrated TCP data
injection attacks, resulting in CVE-2023-52881 assigned by Linux and
the 2023 patch:
Commit 3d501dd326fb1c7 ("tcp: do not accept ACK of bytes we never sent")
that rejects ACKs for bytes so far back that were never sent.

Link: https://www.cve.org/CVERecord?id=CVE-2023-52881

Both RFC5961 and the later patch were only applied to the slow path,
leaving the fast path vulnerable and noncompliant with RFC5961.

Enforce a validation test for the SEG.ACK in the fast path, before the data
is processed. Failure to pass the validation will result in a challenge ACK
and the packet will be discarded in compliance with RFC5961.

Some details:
RFC5961 (and the 2023 patch) is enforced in tcp_ack()
(./net/ipv4/tcp_input.c).
Incoming segments to a socket in ESTABLISHED state are processed in
tcp_rcv_established() (./net/ipv4/tcp_input.c).
Consider a packet that violates RFC5961 (meaning the SEG.ACK is too early).
In the slow path (starting at the label "slow_path"), tcp_ack() is invoked,
well before processing the segment data.
A challenge ACK is sent there, tcp_ack() returns
-SKB_DROP_REASON_TCP_TOO_OLD_ACK,
and slow path discards the segment as expected.
In the fast path, tcp_ack() is also called,
but only after the data from the segment is processed.
Furthermore, the return value from tcp_ack() is not checked.
De-facto, the data from the segment is accepted
(and an ACK is generated), even though the segment violates RFC5961.

The following packetdrill script shows the issue at hand.
Linux (as a server) accepts data segment processed in the fast path with
an ack that is far too low.

// BASED ON PACKETDRILL SCRIPT FROM:
// Commit 3d501dd326fb1c7 ("tcp: do not accept ACK of bytes we never sent")
0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1024) = 0

// ---------------- Handshake ------------------- //
+0 < S 0:0(0) win 65535
+0 > S. 0:0(0) ack 1 <...>
+0 < . 1:1(0) ack 1 win 65535
+0 accept(3, ..., ...) = 4

// Data must be first sent/received on the socket
// so that memory is allocated (sk_forward_alloc should be > 0)
// and later data will be proccessed in the fast path
+0 < P. 1:501(500) ack 1 win 65535 //valid packet forcing memory allocation
+0 > . 1:1(0) ack 501

// incoming segment, ack way in the past... (101 + 2^32 - 1500000000)
// Oops, unpatched kernels happily accept this packet
+0 < P. 501:1501(1000) ack 2794967397 win 65535

// On unpatched kernels, this ACK will match,
// showing that the segment is accepted
+0 > . 1:1(0) ack 1501

Reported-by: Amit Klein <redacted>
Reported-by: Tamir Shahar <redacted>
Reported-by: Inbal Schussheim <redacted>
Signed-off-by: Inbal Schussheim <redacted>
---
 net/ipv4/tcp_input.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index daff93d51342..2474871e80ec 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -4272,6 +4272,17 @@ static void tcp_rack_update_reo_wnd(struct sock *sk, struct rate_sample *rs)
        }
 }

+/* Validates that the ACK is older than the acceptable historical ACK window*/
+static inline bool tcp_ack_too_old(const struct tcp_sock *tp, u32 ack,
+                                  u32 snd_una)
+{
+       u32 max_window;
+
+       max_window = min_t(u64, tp->max_window, tp->bytes_acked);
+
+       return before(ack, snd_una - max_window);
+}
+
 /* This routine deals with incoming acks, but not outgoing ones. */
 static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
 {

@@ -4303,12 +4314,8 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
         * then we can probably ignore it.
         */
        if (before(ack, prior_snd_una)) {
-               u32 max_window;
-
-               /* do not accept ACK for bytes we never sent. */
-               max_window = min_t(u64, tp->max_window, tp->bytes_acked);
                /* RFC 5961 5.2 [Blind Data Injection Attack].[Mitigation] */
-               if (before(ack, prior_snd_una - max_window)) {
+               if (tcp_ack_too_old(tp, ack, prior_snd_una)) {
                        if (!(flag & FLAG_NO_CHALLENGE_ACK))
                                tcp_send_challenge_ack(sk, false);
                        return -SKB_DROP_REASON_TCP_TOO_OLD_ACK;

@@ -6614,6 +6621,15 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
                        if ((int)skb->truesize > sk->sk_forward_alloc)
                                goto step5;

+                       if (unlikely(before(TCP_SKB_CB(skb)->ack_seq, tp->snd_una))) {
+                               if (tcp_ack_too_old(tp, TCP_SKB_CB(skb)->ack_seq,
+                                                   tp->snd_una)) {
+                                       tcp_send_challenge_ack(sk, false);
+                                       reason = SKB_DROP_REASON_TCP_TOO_OLD_ACK;
+                                       goto discard;
+                               }
+                       }
+