Re: [PATCH net v3] tcp: reject completely old segments during sequence validation

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] Incomplete fix: the new old-segment rejection in tcp_sequence()
  (net/ipv4/tcp_input.c:4847-4852) is bypassed by any segment that sets…
- [Low] The block comment documenting tcp_sequence()
  (net/ipv4/tcp_input.c:4827-4838) now contradicts the implementation in…

--

tcp: reject completely old segments during sequence validation

This tightens tcp_sequence() so that a segment consuming sequence space with
end_seq equal to tp->rcv_wup is rejected as completely old, instead of only
rejecting end_seq strictly before rcv_wup.  Segments with SYN or FIN set, and
segments that consume no sequence space, keep the previous behaviour.

[Severity: High]
Can a data segment that also sets FIN still slip through this check?

For a segment with N bytes of payload and FIN set, tcp_v4_rcv() (and
tcp_v6_rcv()) compute:

	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
				    skb->len - th->doff * 4);

so end_seq = seq + N + 1.  Choosing seq = rcv_wup - N - 1 gives
end_seq == tp->rcv_wup, which means:

  - the outer !after(end_seq, tp->rcv_wup) test is true,
  - before(end_seq, tp->rcv_wup) is false,
  - seq != end_seq is true, but the mask term
    !(tcp_flag_byte(th) & (TCPHDR_SYN | TCPHDR_FIN)) is false because FIN
    is set.

tcp_sequence() then returns SKB_NOT_DROPPED_YET.  In
tcp_validate_incoming() the segment is not RST and not SYN, so step 2 and
step 4 are skipped and control reaches pass:, after which
tcp_rcv_established() calls:

	reason = tcp_ack(sk, skb, FLAG_SLOWPATH | FLAG_UPDATE_TS_RECENT);

Isn't that exactly the ACK processing of a completely old segment that the
changelog and the packetdrill reproducer aim to prevent, reachable again by
setting one flag bit?  The v2 note says the carve-out exists to
"preserve existing SYN/FIN handling, including simultaneous connect and
retransmitted SYN+ACK AccECN processing", but a retransmitted pure FIN has
end_seq == seq + 1 with no payload, so would restricting the exemption to
segments that consume no payload sequence space be enough, rather than
exempting all FIN-flagged segments including data+FIN?

Note tcp_data_queue() still drops the payload and does not call tcp_fin()
because seq != rcv_nxt, so the residual effect is the ACK processing itself
(snd_una and bytes_acked accounting, RTT and window sampling, congestion
control state) driven by the peer-controlled ack field of an
RFC-unacceptable segment.

[Severity: Low]
Does the block comment above tcp_sequence() still match the code after this
change?  It currently says:

 * Segment controls are considered valid, if the segment
 * fits to the window after truncation to the window. Acceptability
 * of data (and SYN, FIN, of course) is checked separately.
 * See tcp_data_queue(), for example.
 *
 * Also, controls (RST is main one) are accepted using RCV.WUP instead
 * of RCV.NXT.

The new branch inspects tcp_flag_byte(th) and seq != end_seq, so the
data versus SYN/FIN acceptability decision now happens inside
tcp_sequence() rather than only in tcp_data_queue().

The second part mentions RST as the main control accepted at RCV.WUP, but
the exemption mask lists only TCPHDR_SYN | TCPHDR_FIN.  A RST carrying
payload (allowed by RFC 1122 4.2.2.12) has end_seq = seq + len with no +1,
so seq != end_seq and neither SYN nor FIN is set.  With
seq = rcv_wup - len such a RST now returns
SKB_DROP_REASON_TCP_OLD_SEQUENCE, and tcp_validate_incoming() takes:

	} else if (tcp_reset_check(sk, skb)) {
		goto reset;
	}
	goto discard;

so it no longer reaches step 2 and no longer emits the RFC 5961 challenge
ACK it produced before.  Would it be better to update the comment to say
that at the RCV.WUP boundary only zero-length segments and SYN/FIN segments
are accepted, or to add RST to the flag mask?

+			return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
+	}
 
 	seq_limit = tp->rcv_nxt + tcp_max_receive_window(tp);
 	if (unlikely(after(end_seq, seq_limit))) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831205611.2439538-1-michael.cohen3%40mail.huji.ac.il