[PATCH 0/4 nf v2] netfilter: synproxy: timestamp adjustment fixes

[PATCH 0/4 nf v2] 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. The most critical bug is the write to the stale
pointer.

FWIW; I am sending these fixes as separated patches because they
are addressing independent issues.

Fernando Fernandez Mancera (4):
  netfilter: synproxy: drop packets if timestamp adjustment fails
  netfilter: synproxy: adjust duplicate timestamp options
  netfilter: synproxy: fix unaligned memory access in timestamp
    adjustment
  netfilter: synproxy: fix possible write to stale pointer

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

-- 
2.53.0

[PATCH 1/4 nf v2] 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 due to ENOMEM from
skb_ensure_writable(). In addition, if there are malformed options, skip
the tstamp update but do not drop the packet as that should be done by
the policy directly.

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..e523b64bf839 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 true;
 			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_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, ENOMEM);
+
 	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_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, ENOMEM);
+
 	return NF_ACCEPT;
 }
 EXPORT_SYMBOL_GPL(ipv6_synproxy_hook);
-- 
2.53.0

[PATCH 2/4 nf v2] netfilter: synproxy: adjust duplicate 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. Let's just continue parsing the following options and in case a
duplicated timestamp is found, adjust it too.

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

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index e523b64bf839..6bd63f5ab75d 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -229,7 +229,6 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 				}
 				inet_proto_csum_replace4(&th->check, skb,
 							 old, *ptr, false);
-				return true;
 			}
 			optoff += op[1];
 		}
-- 
2.53.0

[PATCH 3/4 nf v2] 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 6bd63f5ab75d..5413133a42fa 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -189,7 +189,7 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 		       const struct nf_conn_synproxy *synproxy)
 {
 	unsigned int optoff, optend;
-	__be32 *ptr, old;
+	u32 new, old;
 
 	if (synproxy->tsoff == 0)
 		return true;
@@ -217,18 +217,17 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 			if (op[0] == TCPOPT_TIMESTAMP &&
 			    op[1] == TCPOLEN_TIMESTAMP) {
 				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);
 			}
 			optoff += op[1];
 		}
-- 
2.53.0

[PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Fernando Fernandez Mancera]

skb_ensure_writable() is called to guarantee that the TCP options area
can be safely modified when adjusting the timestamp. As it expands or
linearize the skb head it might reallocate the data buffer.

This makes the th pointer passed by the caller stale. The following
writes to the TCP header might be done to a stale pointer.

Recalculating the th pointer after skb_ensure_writable() prevents this
issue from happening.

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

diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 5413133a42fa..84d36d4367a6 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -199,6 +199,7 @@ synproxy_tstamp_adjust(struct sk_buff *skb, unsigned int protoff,
 
 	if (skb_ensure_writable(skb, optend))
 		return false;
+	th = (struct tcphdr *)(skb->data + protoff);
 
 	while (optoff < optend) {
 		unsigned char *op = skb->data + optoff;
-- 
2.53.0

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Fernando Fernandez Mancera]

On 5/25/26 2:44 PM, Fernando Fernandez Mancera wrote:
> skb_ensure_writable() is called to guarantee that the TCP options area
> can be safely modified when adjusting the timestamp. As it expands or
> linearize the skb head it might reallocate the data buffer.
> 
> This makes the th pointer passed by the caller stale. The following
> writes to the TCP header might be done to a stale pointer.
> 
> Recalculating the th pointer after skb_ensure_writable() prevents this
> issue from happening.
> 
> Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>

LOL. I just realized I already reviewed this at:

https://lore.kernel.org/netfilter-devel/20260522104257.2008-3-fw@strlen.de/T/#u

*facepalm* sorry for the noise, Florian could you ignore this patch but 
consider the other 3 fixes?

Thanks and I apologize again!
Fernando.

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 5/25/26 2:44 PM, Fernando Fernandez Mancera wrote:
> > skb_ensure_writable() is called to guarantee that the TCP options area
> > can be safely modified when adjusting the timestamp. As it expands or
> > linearize the skb head it might reallocate the data buffer.
> > 
> > This makes the th pointer passed by the caller stale. The following
> > writes to the TCP header might be done to a stale pointer.
> > 
> > Recalculating the th pointer after skb_ensure_writable() prevents this
> > issue from happening.
> > 
> > Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
> > Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> 
> LOL. I just realized I already reviewed this at:
> 
> https://lore.kernel.org/netfilter-devel/20260522104257.2008-3-fw@strlen.de/T/#u
> 
> *facepalm* sorry for the noise, Florian could you ignore this patch but 
> consider the other 3 fixes?

I know its tiresome, but would you mind sending a new version that also
fixes up the other things pointed out by sashiko?

https://sashiko.dev/#/patchset/20260525124450.6043-1-fmancera%40suse.de

In particular, seqadj and concurrent registration.  As these bugs aren't
as severe as patch 4, I think nf-next would be fine as well.

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Fernando Fernandez Mancera]

On 5/25/26 8:59 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 5/25/26 2:44 PM, Fernando Fernandez Mancera wrote:
>>> skb_ensure_writable() is called to guarantee that the TCP options area
>>> can be safely modified when adjusting the timestamp. As it expands or
>>> linearize the skb head it might reallocate the data buffer.
>>>
>>> This makes the th pointer passed by the caller stale. The following
>>> writes to the TCP header might be done to a stale pointer.
>>>
>>> Recalculating the th pointer after skb_ensure_writable() prevents this
>>> issue from happening.
>>>
>>> Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
>>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>>
>> LOL. I just realized I already reviewed this at:
>>
>> https://lore.kernel.org/netfilter-devel/20260522104257.2008-3-fw@strlen.de/T/#u
>>
>> *facepalm* sorry for the noise, Florian could you ignore this patch but
>> consider the other 3 fixes?
> 
> I know its tiresome, but would you mind sending a new version that also
> fixes up the other things pointed out by sashiko?
> 
> https://sashiko.dev/#/patchset/20260525124450.6043-1-fmancera%40suse.de

Ohno, more stuff. Sure I will look into it.

> 
> In particular, seqadj and concurrent registration.  As these bugs aren't
> as severe as patch 4, I think nf-next would be fine as well.
> 

Great, yes, that was my original idea.. routing a v3 for nf-next.

Thanks Florian!

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Fernando Fernandez Mancera]

On 5/25/26 8:59 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 5/25/26 2:44 PM, Fernando Fernandez Mancera wrote:
>>> skb_ensure_writable() is called to guarantee that the TCP options area
>>> can be safely modified when adjusting the timestamp. As it expands or
>>> linearize the skb head it might reallocate the data buffer.
>>>
>>> This makes the th pointer passed by the caller stale. The following
>>> writes to the TCP header might be done to a stale pointer.
>>>
>>> Recalculating the th pointer after skb_ensure_writable() prevents this
>>> issue from happening.
>>>
>>> Fixes: 48b1de4c110a ("netfilter: add SYNPROXY core/target")
>>> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
>>
>> LOL. I just realized I already reviewed this at:
>>
>> https://lore.kernel.org/netfilter-devel/20260522104257.2008-3-fw@strlen.de/T/#u
>>
>> *facepalm* sorry for the noise, Florian could you ignore this patch but
>> consider the other 3 fixes?
> 
> I know its tiresome, but would you mind sending a new version that also
> fixes up the other things pointed out by sashiko?
> 
> https://sashiko.dev/#/patchset/20260525124450.6043-1-fmancera%40suse.de
> 
> In particular, seqadj and concurrent registration.  As these bugs aren't
> as severe as patch 4, I think nf-next would be fine as well.
> 
> 

I have been taking a look to this. The seqadj issue isn't real IMHO.

The logic here is protected by the TCP state machine. AFAIU, ALGs only 
look at fully established connections, I don't see how this can happen 
even for retransmitted packets, the TCP state would remain the same so 
nf_ct_seqadj_init() should never race with nf_ct_seqadj_set().

FWIW; the concurrent registration issue is real. I am writing a fix.

Thanks Florian!

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> > In particular, seqadj and concurrent registration.  As these bugs aren't
> > as severe as patch 4, I think nf-next would be fine as well.
> > 
> > 
> 
> I have been taking a look to this. The seqadj issue isn't real IMHO.

We could be processing a 2nd packet in parallel, in particular reply
direction is a concern.

> The logic here is protected by the TCP state machine. AFAIU, ALGs only 
> look at fully established connections, I don't see how this can happen 
> even for retransmitted packets, the TCP state would remain the same so 
> nf_ct_seqadj_init() should never race with nf_ct_seqadj_set().

Can't we race with nf_ct_seq_adjust(), e.g. for closed state when
connection is reopened?

> FWIW; the concurrent registration issue is real. I am writing a fix.

Thanks.

Re: [PATCH 4/4 nf v2] netfilter: synproxy: fix possible write to stale pointer

[Fernando Fernandez Mancera]

On 5/26/26 3:05 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>>> In particular, seqadj and concurrent registration.  As these bugs aren't
>>> as severe as patch 4, I think nf-next would be fine as well.
>>>
>>>
>>
>> I have been taking a look to this. The seqadj issue isn't real IMHO.
> 
> We could be processing a 2nd packet in parallel, in particular reply
> direction is a concern.
> 

Ugh. I missed that. Yes, then it seems real. Let me wrap it with proper 
locking..

Thanks Florian and sorry for the noise :)