netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

[PATCH] netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

Posted by Rosen Penev 1 week, 6 days ago

The timestamp-only fast path dereferences the option stream as
*(__be32 *)ptr, which assumes 4-byte alignment that the TCP option
stream does not guarantee. Use get_unaligned_be32() instead, which
reads the value safely and already returns host byte order, so the
htonl() on the comparison constant can be dropped.

This matches the existing get_unaligned_be32() use later in the same
function.

Assisted-by: Claude:Opus-4.7
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index b67426c2189b..8993374c9df2 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -405,11 +405,11 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 		return;
 
 	/* Fast path for timestamp-only option */
-	if (length == TCPOLEN_TSTAMP_ALIGNED
-	    && *(__be32 *)ptr == htonl((TCPOPT_NOP << 24)
-				       | (TCPOPT_NOP << 16)
-				       | (TCPOPT_TIMESTAMP << 8)
-				       | TCPOLEN_TIMESTAMP))
+	if (length == TCPOLEN_TSTAMP_ALIGNED &&
+	    get_unaligned_be32(ptr) == ((TCPOPT_NOP << 24) |
+					(TCPOPT_NOP << 16) |
+					(TCPOPT_TIMESTAMP << 8) |
+					TCPOLEN_TIMESTAMP))
 		return;
 
 	while (length > 0) {
-- 
2.54.0

Re: [PATCH] netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

Posted by Pablo Neira Ayuso 1 day, 12 hours ago

On Mon, May 25, 2026 at 02:58:40PM -0700, Rosen Penev wrote:
> The timestamp-only fast path dereferences the option stream as
> *(__be32 *)ptr, which assumes 4-byte alignment that the TCP option
> stream does not guarantee. Use get_unaligned_be32() instead, which
> reads the value safely and already returns host byte order, so the
> htonl() on the comparison constant can be dropped.
> 
> This matches the existing get_unaligned_be32() use later in the same
> function.
> 
> Assisted-by: Claude:Opus-4.7
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index b67426c2189b..8993374c9df2 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -405,11 +405,11 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
>  		return;
>  
>  	/* Fast path for timestamp-only option */
> -	if (length == TCPOLEN_TSTAMP_ALIGNED
> -	    && *(__be32 *)ptr == htonl((TCPOPT_NOP << 24)
> -				       | (TCPOPT_NOP << 16)
> -				       | (TCPOPT_TIMESTAMP << 8)
> -				       | TCPOLEN_TIMESTAMP))
> +	if (length == TCPOLEN_TSTAMP_ALIGNED &&
> +	    get_unaligned_be32(ptr) == ((TCPOPT_NOP << 24) |
> +					(TCPOPT_NOP << 16) |
> +					(TCPOPT_TIMESTAMP << 8) |
> +					TCPOLEN_TIMESTAMP))

Missing put_unaligned_be32(), BTW.

>  		return;
>  
>  	while (length > 0) {
> -- 
> 2.54.0
>

Re: [PATCH] netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

Posted by Pablo Neira Ayuso 1 day, 12 hours ago

On Sun, Jun 07, 2026 at 11:09:43AM +0200, Pablo Neira Ayuso wrote:
> On Mon, May 25, 2026 at 02:58:40PM -0700, Rosen Penev wrote:
> > The timestamp-only fast path dereferences the option stream as
> > *(__be32 *)ptr, which assumes 4-byte alignment that the TCP option
> > stream does not guarantee. Use get_unaligned_be32() instead, which
> > reads the value safely and already returns host byte order, so the
> > htonl() on the comparison constant can be dropped.
> > 
> > This matches the existing get_unaligned_be32() use later in the same
> > function.
> > 
> > Assisted-by: Claude:Opus-4.7
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  net/netfilter/nf_conntrack_proto_tcp.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index b67426c2189b..8993374c9df2 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -405,11 +405,11 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
> >  		return;
> >  
> >  	/* Fast path for timestamp-only option */
> > -	if (length == TCPOLEN_TSTAMP_ALIGNED
> > -	    && *(__be32 *)ptr == htonl((TCPOPT_NOP << 24)
> > -				       | (TCPOPT_NOP << 16)
> > -				       | (TCPOPT_TIMESTAMP << 8)
> > -				       | TCPOLEN_TIMESTAMP))
> > +	if (length == TCPOLEN_TSTAMP_ALIGNED &&
> > +	    get_unaligned_be32(ptr) == ((TCPOPT_NOP << 24) |
> > +					(TCPOPT_NOP << 16) |
> > +					(TCPOPT_TIMESTAMP << 8) |
> > +					TCPOLEN_TIMESTAMP))
> 
> Missing put_unaligned_be32(), BTW.

Sorry, no write in this case, only read, LGTM. Apologies.

> >  		return;
> >  
> >  	while (length > 0) {
> > -- 
> > 2.54.0
> >

Re: [PATCH] netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

Posted by Fernando Fernandez Mancera 1 week, 6 days ago

On 5/25/26 11:58 PM, Rosen Penev wrote:
> The timestamp-only fast path dereferences the option stream as
> *(__be32 *)ptr, which assumes 4-byte alignment that the TCP option
> stream does not guarantee. Use get_unaligned_be32() instead, which
> reads the value safely and already returns host byte order, so the
> htonl() on the comparison constant can be dropped.
> 
> This matches the existing get_unaligned_be32() use later in the same
> function.
> 
> Assisted-by: Claude:Opus-4.7
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
I already spotted this corner case when working on a SYNPROXY patch [1] 
but didn't send a patch yet. I think this is for correctness too.

Anyway, it is likely that there are more places where this tweak is 
needed.. I will look around.. meanwhile:

Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>

[1] lore.kernel.org/netfilter-devel/20260525124450.6043-4-fmancera@suse.de/

Re: [PATCH] netfilter: nf_conntrack: use get_unaligned_be32() in tcp_sack()

Posted by Pablo Neira Ayuso 1 day, 12 hours ago

Hi Fernando,

On Tue, May 26, 2026 at 12:35:22AM +0200, Fernando Fernandez Mancera wrote:
> On 5/25/26 11:58 PM, Rosen Penev wrote:
> > The timestamp-only fast path dereferences the option stream as
> > *(__be32 *)ptr, which assumes 4-byte alignment that the TCP option
> > stream does not guarantee. Use get_unaligned_be32() instead, which
> > reads the value safely and already returns host byte order, so the
> > htonl() on the comparison constant can be dropped.
> > 
> > This matches the existing get_unaligned_be32() use later in the same
> > function.
> > 
> > Assisted-by: Claude:Opus-4.7
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> I already spotted this corner case when working on a SYNPROXY patch [1] but
> didn't send a patch yet. I think this is for correctness too.
> 
> Anyway, it is likely that there are more places where this tweak is needed..

I agree a more general audit to spot unaligned access, targetting
nf-next would be good.

Thanks.

> I will look around.. meanwhile:
> 
> Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>
> 
> [1] lore.kernel.org/netfilter-devel/20260525124450.6043-4-fmancera@suse.de/