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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sun, Jun 21, 2026 at 07:49:33PM +0100, Kacper Kokot wrote:
> 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@gmail.com>
> ---
> 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);
> +			}

After seeing this unaligned in other areas in the Netfilter tree, I am
not sure it is worth to add workarounds everywhere in this codebase to
deal with updates that span two 16-bits words for such a hypothetical
case like this.

By now, patches that call get_unaligned_be16() for correctness are OK
IMO. This is to deal with arches which cannot cope with unaligned
access. This will corrupt such rare packet but that it addresses the
unaligned splats.

If we start seeing real stacks which provide real unaligned access
like this, maybe by then we can revisit.

So I am leaning towards a small patches to introduce
get_unaligned_be16() and document that this corrupts packets with such
a rare unaligned TCP option.

IIRC, x86_64 has a inet checksum function that can deal with 1-byte
words, although other arches cannot do that and still need to
operation with 16-bit words. Given Linux is multi-arch, this all need
to stick to the 16-bit word arithmetics when mangling packets

Maybe in the future all checksum functions in every arch are updated
too to deal with 1-byte word updates, and maybe real stacks pop up
with such a rare packets. But by then these ugly workaround won't be
needed at all.

> +
>  			inet_proto_csum_replace2(&tcph->check, skb,
> -						 htons(oldmss), htons(newmss),
> +						 csum_oldmss, csum_newmss,
>  						 false);
>  			return 0;
>  		}
> -- 
> 2.43.0
> 
>