[PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Fernando Fernandez Mancera]

This series fixes several long standing issues during synproxy timestamp
adjustment. From ignored error handling to unaligned memory access. Most
of this are not issues impacting real setups as they would have been
reported before.

I targeted nf-next tree as they are fixes for correctness.

Fernando Fernandez Mancera (3):
  netfilter: synproxy: drop packets if timestamp adjustment fails
  netfilter: synproxy: drop packets with duplicated timestamp options
  netfilter: synproxy: fix unaligned memory access in timestamp
    adjustment

 net/netfilter/nf_synproxy_core.c | 47 +++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH 1/3 nf-next] netfilter: synproxy: drop packets if timestamp adjustment fails

[Fernando Fernandez Mancera]

If a packet was malformed or if skb_ensure_writable() failed, the
synproxy_tstamp_adjust() function returned 0 indicating an error but it
was ignored on the callers.

Make the function return a boolean instead to clarify the result and
drop the packet if synproxy_tstamp_adjust() failed.

Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/netfilter/nf_synproxy_core.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 57f57e2fc80a..51a3dd48995b 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -182,7 +182,7 @@ synproxy_check_timestamp_cookie(struct synproxy_options *opts)
 	opts->options |= opts->tsecr & (1 << 5) ? NF_SYNPROXY_OPT_ECN : 0;
 }
 
-static unsigned int
+static bool
 synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 		       struct tcphdr *th, struct nf_conn *ct,
 		       enum ip_conntrack_info ctinfo,
@@ -192,20 +192,20 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 	__be32 *ptr, old;
 
 	if (synproxy->tsoff == 0)
-		return 1;
+		return true;
 
 	optoff = protoff + sizeof(struct tcphdr);
 	optend = protoff + th->doff * 4;
 
 	if (skb_ensure_writable(skb, optend))
-		return 0;
+		return false;
 
 	while (optoff < optend) {
 		unsigned char *op = skb->data + optoff;
 
 		switch (op[0]) {
 		case TCPOPT_EOL:
-			return 1;
+			return true;
 		case TCPOPT_NOP:
 			optoff++;
 			continue;
@@ -213,7 +213,7 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 			if (optoff + 1 == optend ||
 			    optoff + op[1] > optend ||
 			    op[1] < 2)
-				return 0;
+				return false;
 			if (op[0] == TCPOPT_TIMESTAMP &&
 			    op[1] == TCPOLEN_TIMESTAMP) {
 				if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY) {
@@ -229,12 +229,12 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 				}
 				inet_proto_csum_replace4(&th->check, skb,
 							 old, *ptr, false);
-				return 1;
+				return true;
 			}
 			optoff += op[1];
 		}
 	}
-	return 1;
+	return true;
 }
 
 #ifdef CONFIG_PROC_FS
@@ -745,7 +745,9 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 		break;
 	}
 
-	synproxy_tstamp_adjust(skb, thoff, th, ct, ctinfo, synproxy);
+	if (!synproxy_tstamp_adjust(skb, thoff, th, ct, ctinfo, synproxy))
+		return NF_DROP;
+
 	return NF_ACCEPT;
 }
 EXPORT_SYMBOL_GPL(ipv4_synproxy_hook);
@@ -1168,7 +1170,9 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 		break;
 	}
 
-	synproxy_tstamp_adjust(skb, thoff, th, ct, ctinfo, synproxy);
+	if (!synproxy_tstamp_adjust(skb, thoff, th, ct, ctinfo, synproxy))
+		return NF_DROP;
+
 	return NF_ACCEPT;
 }
 EXPORT_SYMBOL_GPL(ipv6_synproxy_hook);
-- 
2.53.0

[PATCH 2/3 nf-next] netfilter: synproxy: drop packets with duplicated timestamp options

[Fernando Fernandez Mancera]

RFC 9293 does not mention anything about duplicated options and each
networking stack handles it in their own way. Currently, Linux kernel is
processing options sequentially and in case of duplicated timestamp
options, the value from the latest one overrides the others.

As SYNPROXY is modifying only the first timestamp option found, a packet
can reach the backend server and it might parse the wrong timestamp
value.  As there is not a use-case for duplicated timestamp option, drop
the packet directly when such situation is found.

Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/netfilter/nf_synproxy_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 51a3dd48995b..49ce18f9c8ef 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -189,6 +189,7 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 		       const struct nf_conn_synproxy *synproxy)
 {
 	unsigned int optoff, optend;
+	bool tstamp_seen = false;
 	__be32 *ptr, old;
 
 	if (synproxy->tsoff == 0)
@@ -216,6 +217,8 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 				return false;
 			if (op[0] == TCPOPT_TIMESTAMP &&
 			    op[1] == TCPOLEN_TIMESTAMP) {
+				if (tstamp_seen)
+					return false;
 				if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY) {
 					ptr = (__be32 *)&op[2];
 					old = *ptr;
@@ -229,7 +232,10 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 				}
 				inet_proto_csum_replace4(&th->check, skb,
 							 old, *ptr, false);
-				return true;
+				/* continue parsing options in case there is a
+				 * duplicated tstamp, drop the packet
+				 */
+				tstamp_seen = true;
 			}
 			optoff += op[1];
 		}
-- 
2.53.0

[PATCH 3/3 nf-next] netfilter: synproxy: fix unaligned memory access in timestamp adjustment

[Fernando Fernandez Mancera]

Use get_unaligned_be32() and put_unaligned_be32() to safely read and
write the timestamp fields. This prevents performance degradation due to
unaligned memory access or even a crash on strict alignment
architectures.

This follows the implementation of timestamp parsing in the networking
stack at tcp_parse_options() and synproxy_parse_options().

Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/netfilter/nf_synproxy_core.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 49ce18f9c8ef..a49124e8e552 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -190,7 +190,7 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 {
 	unsigned int optoff, optend;
 	bool tstamp_seen = false;
-	__be32 *ptr, old;
+	u32 new, old;
 
 	if (synproxy->tsoff == 0)
 		return true;
@@ -220,18 +220,17 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 				if (tstamp_seen)
 					return false;
 				if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY) {
-					ptr = (__be32 *)&op[2];
-					old = *ptr;
-					*ptr = htonl(ntohl(*ptr) -
-						     synproxy->tsoff);
+					old = get_unaligned_be32(&op[2]);
+					new = old - synproxy->tsoff;
+					put_unaligned_be32(new, &op[2]);
 				} else {
-					ptr = (__be32 *)&op[6];
-					old = *ptr;
-					*ptr = htonl(ntohl(*ptr) +
-						     synproxy->tsoff);
+					old = get_unaligned_be32(&op[6]);
+					new = old + synproxy->tsoff;
+					put_unaligned_be32(new, &op[6]);
 				}
 				inet_proto_csum_replace4(&th->check, skb,
-							 old, *ptr, false);
+							 cpu_to_be32(old),
+							 cpu_to_be32(new), false);
 				/* continue parsing options in case there is a
 				 * duplicated tstamp, drop the packet
 				 */
-- 
2.53.0

Re: [PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Fernando Fernandez Mancera]

On 5/23/26 9:47 PM, Fernando Fernandez Mancera wrote:
> This series fixes several long standing issues during synproxy timestamp
> adjustment. From ignored error handling to unaligned memory access. Most
> of this are not issues impacting real setups as they would have been
> reported before.
> 
> I targeted nf-next tree as they are fixes for correctness.
> 

FWIW; I am sending a v2 but I am not sure if it should go to nf tree as 
it will include a fix to a UAF spotted in the same code so I guess it 
makes sense to merge it together.

See:

https://sashiko.dev/#/patchset/20260523194743.5888-2-fmancera%40suse.de

What do you all think? nf or nf-next for the 4 commits? I tried to 
reproduce an UAF but couldn't trigger it. Although, I was able to write 
to the stale pointer using tc mirred..

Thanks,
Fernando.

> Fernando Fernandez Mancera (3):
>    netfilter: synproxy: drop packets if timestamp adjustment fails
>    netfilter: synproxy: drop packets with duplicated timestamp options
>    netfilter: synproxy: fix unaligned memory access in timestamp
>      adjustment
> 
>   net/netfilter/nf_synproxy_core.c | 47 +++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 19 deletions(-)
> 

Re: [PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> What do you all think? nf or nf-next for the 4 commits? I tried to reproduce
> an UAF but couldn't trigger it. Although, I was able to write to the stale
> pointer using tc mirred..

nf.  But I'm not sold on this series: dropping patckets outside
of rulesets should be a last resort.

Re: [PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Fernando Fernandez Mancera]

On 5/24/26 7:16 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> What do you all think? nf or nf-next for the 4 commits? I tried to reproduce
>> an UAF but couldn't trigger it. Although, I was able to write to the stale
>> pointer using tc mirred..
> 
> nf.  But I'm not sold on this series: dropping patckets outside
> of rulesets should be a last resort.

Hm. That might be an appealing argument for the dup timestamp option, if 
we want to be more relaxed we can just adjust all timestamp options 
equally (we need to assume that this is a corner case of course).

But when failing skb_ensure_writable() or when encountering completely 
malformed options I believe we should drop the packet. Because either we 
didn't adjust the timestamp properly or the options are just malformed 
and in any of these cases the packet should reach the backend..

Thanks Florian!

Re: [PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> Hm. That might be an appealing argument for the dup timestamp option, if we
> want to be more relaxed we can just adjust all timestamp options equally (we
> need to assume that this is a corner case of course).
> 
> But when failing skb_ensure_writable() or when encountering completely
> malformed options I believe we should drop the packet.

Not so sure. skb_ensure_writable() -> yes.
But for malformed options?  I think it should be done by policy.
Not even conntrack drops such packets at the moment.

Could you use NF_DROP_REASON() in next version?
NF_DROP conceals the drop location which makes debugging harder.

Thanks!

Re: [PATCH 0/3 nf-next] netfilter: synproxy: timestamp adjustment fixes

[Fernando Fernandez Mancera]

On 5/24/26 9:24 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> Hm. That might be an appealing argument for the dup timestamp option, if we
>> want to be more relaxed we can just adjust all timestamp options equally (we
>> need to assume that this is a corner case of course).
>>
>> But when failing skb_ensure_writable() or when encountering completely
>> malformed options I believe we should drop the packet.
> 
> Not so sure. skb_ensure_writable() -> yes.
> But for malformed options?  I think it should be done by policy.
> Not even conntrack drops such packets at the moment.
> 

Hm, okay that makes sense I didn't know that. I agree with you then. 
Thanks for explaining.

> Could you use NF_DROP_REASON() in next version?
> NF_DROP conceals the drop location which makes debugging harder.
> 

I was also not aware of this. It makes a lot of sense.

Thanks,
Fernando.