[PATCH nf-next v3] netfilter: TCPMSS: handle packets with unaligned MSS option

[Kacper Kokot]

RFC 9293 permits TCP options to begin on any octet boundary. Padding
to a word boundary with NOPs is a sender convention, not a requirement,
and robust receivers must handle unaligned options (MUST-64).

The xt_TCPMSS target's incremental checksum update assumes the MSS
option is word-aligned. When it's not, the modified bytes straddle
two checksum words and the resulting checksum is incorrect. The mangled
packet may then fail checksum validation and be dropped downstream.
That said, all mainstream stacks emit a word-aligned MSS, this change is
motivated by spec conformance rather than a bug observed in the wild.

Extend the checksum update to handle unaligned MSS options. When the
changed word is unaligned, the modified bytes b' and c' straddle two
checksum words w1 and w2:

    | w1     | w2     |
OLD |  a  b  |  c  d  |
NEW |  a  b' |  c' d  |

The two-step update C' = C - w1 + w1' - w2 + w2' reduces algebraically
to a single word incremental checksum update with byteswapped operands:

    C' = C - w1 - w2 + w1' + w2'
       = C - (a * 2^8 + b)  - (c * 2^8 + d)
           + (a * 2^8 + b') + (c' * 2^8 + d)
       = C + 2^8 * (a - a + c' - c) + (b' - b + d - d)
       = C + 2^8 * (c' - c) + (b' - b)
       = C - (2^8 * c + b) + (2^8 * c' + b')

So the unaligned case adds no extra checksum operations.

Signed-off-by: Kacper Kokot <kacper.kokot.44@xxxxxxxxx>
---
v3:
 - Reframe as enhancement, not a fix (Pablo/Fernando)
 - Rename subject to xt_TCPMSS, drop "fix" wording
 - Reword commit message: packet may fail checksum validation and be
   dropped downstream (Pablo)
 - Target nf-next (Fernando)
 - Use __be16 for csum_oldmss/csum_newmss (sparse warning from
   kernel test robot)
 - Reorder local variable declarations to reverse xmas tree (Fernando)

v2:
 - Use get_unaligned_be16 (Fernando's suggestion)
 - Fix alignment check expression (David)
 - Mention it's a theoretical bug in the commit message
 - Drop cc stable, the bug is only theoretical

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 80e1634bc51f..037add799d41 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -116,9 +116,10 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	opt = (u_int8_t *)tcph;
 	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
 		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
+			__be16 csum_oldmss, csum_newmss;
 			u_int16_t oldmss;
 
-			oldmss = (opt[i+2] << 8) | opt[i+3];
+			oldmss = get_unaligned_be16(&opt[i + 2]);
 
 			/* Never increase MSS, even when setting it, as
 			 * doing so results in problems for hosts that rely
@@ -130,8 +131,25 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 			opt[i+2] = (newmss & 0xff00) >> 8;
 			opt[i+3] = newmss & 0x00ff;
 
+			csum_oldmss = htons(oldmss);
+			csum_newmss = htons(newmss);
+
+			if (((char *)&opt[i + 2] - (char *)tcph) & 0x1) {
+				/* MSS option is unaligned: the modified bytes
+				 * straddle two checksum words. Byteswapping
+				 * the operands lets a single incremental
+				 * update produce the correct checksum delta
+				 * (see commit message for the derivation).
+				 */
+				csum_oldmss = htons(swab16(oldmss));
+				csum_newmss = htons(swab16(newmss));
+			} else {
+				csum_oldmss = htons(oldmss);
+				csum_newmss = htons(newmss);
+			}
+
 			inet_proto_csum_replace2(&tcph->check, skb,
-						 htons(oldmss), htons(newmss),
+						 csum_oldmss, csum_newmss,
 						 false);
 			return 0;
 		}
-- 
2.43.0