[PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Fernando Fernandez Mancera]

For lshift and rshift, the shift operations are performed in a loop over
32-bit words. The loop calculates the shifted value and write it to dst,
and then immediately reads from src to calculate the carry for the next
iteration. Because src and dst could point to the same memory location,
the carry is incorrectly calculated using the newly modified dst value
instead of the original src value.

Adding a temporary local variable to cache the original value before
writing to dst and using it for the carry calculation solves the
problem. In addition, partial overlap is rejected from control plane for
all kind of operations. This was tested with the following bytecode:

table test_table ip flags 0 use 1 handle 1
ip test_table test_chain use 3 type filter hook input prio 0 policy accept packets 0 bytes 0 flags 1
ip test_table test_chain 2
  [ immediate reg 1 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
  [ cmp eq reg 1 0x66443322 0x00887766 ]
  [ counter pkts 0 bytes 0 ]
ip test_table test_chain 4 3
  [ immediate reg 1 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
  [ cmp eq reg 1 0x55443322 0x00887766 ]
  [ counter pkts 21794 bytes 1917798 ]

Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: handled partially register overlap
v3: reject partially overlap from control plane
v4: applied the partial overlap check to all operations
---
 net/netfilter/nft_bitwise.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 13808e9cd999..76e7ae96429d 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -43,8 +43,10 @@ static void nft_bitwise_eval_lshift(u32 *dst, const u32 *src,
 	u32 carry = 0;
 
 	for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
-		dst[i - 1] = (src[i - 1] << shift) | carry;
-		carry = src[i - 1] >> (BITS_PER_TYPE(u32) - shift);
+		u32 tmp_src = src[i - 1];
+
+		dst[i - 1] = (tmp_src << shift) | carry;
+		carry = tmp_src >> (BITS_PER_TYPE(u32) - shift);
 	}
 }
 
@@ -56,8 +58,10 @@ static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
 	u32 carry = 0;
 
 	for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
-		dst[i] = carry | (src[i] >> shift);
-		carry = src[i] << (BITS_PER_TYPE(u32) - shift);
+		u32 tmp_src = src[i];
+
+		dst[i] = carry | (tmp_src >> shift);
+		carry = tmp_src << (BITS_PER_TYPE(u32) - shift);
 	}
 }
 
@@ -244,6 +248,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 			    const struct nlattr * const tb[])
 {
 	struct nft_bitwise *priv = nft_expr_priv(expr);
+	unsigned int n;
 	u32 len;
 	int err;
 
@@ -264,6 +269,12 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
+	n = DIV_ROUND_UP(priv->len, sizeof(u32));
+	if (priv->sreg != priv->dreg &&
+	    priv->dreg < priv->sreg + n &&
+	    priv->sreg < priv->dreg + n)
+		return -EINVAL;
+
 	if (tb[NFTA_BITWISE_OP]) {
 		priv->op = ntohl(nla_get_be32(tb[NFTA_BITWISE_OP]));
 		switch (priv->op) {
-- 
2.53.0

Re: [PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Pablo Neira Ayuso]

On Mon, Apr 27, 2026 at 11:21:18AM +0200, Fernando Fernandez Mancera wrote:
> For lshift and rshift, the shift operations are performed in a loop over
> 32-bit words. The loop calculates the shifted value and write it to dst,
> and then immediately reads from src to calculate the carry for the next
> iteration. Because src and dst could point to the same memory location,
> the carry is incorrectly calculated using the newly modified dst value
> instead of the original src value.
> 
> Adding a temporary local variable to cache the original value before
> writing to dst and using it for the carry calculation solves the
> problem. In addition, partial overlap is rejected from control plane for
> all kind of operations. This was tested with the following bytecode:
> 
> table test_table ip flags 0 use 1 handle 1
> ip test_table test_chain use 3 type filter hook input prio 0 policy accept packets 0 bytes 0 flags 1
> ip test_table test_chain 2
>   [ immediate reg 1 0x44332211 0x88776655 ]
>   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
>   [ cmp eq reg 1 0x66443322 0x00887766 ]
>   [ counter pkts 0 bytes 0 ]
> ip test_table test_chain 4 3
>   [ immediate reg 1 0x44332211 0x88776655 ]
>   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
>   [ cmp eq reg 1 0x55443322 0x00887766 ]
>   [ counter pkts 21794 bytes 1917798 ]
> 
> Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
> v2: handled partially register overlap
> v3: reject partially overlap from control plane
> v4: applied the partial overlap check to all operations
> ---
>  net/netfilter/nft_bitwise.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> index 13808e9cd999..76e7ae96429d 100644
> --- a/net/netfilter/nft_bitwise.c
> +++ b/net/netfilter/nft_bitwise.c
> @@ -43,8 +43,10 @@ static void nft_bitwise_eval_lshift(u32 *dst, const u32 *src,
>  	u32 carry = 0;
>  
>  	for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
> -		dst[i - 1] = (src[i - 1] << shift) | carry;
> -		carry = src[i - 1] >> (BITS_PER_TYPE(u32) - shift);
> +		u32 tmp_src = src[i - 1];
> +
> +		dst[i - 1] = (tmp_src << shift) | carry;
> +		carry = tmp_src >> (BITS_PER_TYPE(u32) - shift);
>  	}
>  }
>  
> @@ -56,8 +58,10 @@ static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
>  	u32 carry = 0;
>  
>  	for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
> -		dst[i] = carry | (src[i] >> shift);
> -		carry = src[i] << (BITS_PER_TYPE(u32) - shift);
> +		u32 tmp_src = src[i];
> +
> +		dst[i] = carry | (tmp_src >> shift);
> +		carry = tmp_src << (BITS_PER_TYPE(u32) - shift);
>  	}
>  }
>  
> @@ -244,6 +248,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
>  			    const struct nlattr * const tb[])
>  {
>  	struct nft_bitwise *priv = nft_expr_priv(expr);
> +	unsigned int n;
>  	u32 len;
>  	int err;
>  
> @@ -264,6 +269,12 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
>  	if (err < 0)
>  		return err;
>  
> +	n = DIV_ROUND_UP(priv->len, sizeof(u32));
> +	if (priv->sreg != priv->dreg &&
> +	    priv->dreg < priv->sreg + n &&
> +	    priv->sreg < priv->dreg + n)
> +		return -EINVAL;

In some cases, there is also sreg2 that probably needs to be handled
too.

Re: [PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Pablo Neira Ayuso]

On Thu, Apr 30, 2026 at 08:11:07AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Apr 27, 2026 at 11:21:18AM +0200, Fernando Fernandez Mancera wrote:
> > For lshift and rshift, the shift operations are performed in a loop over
> > 32-bit words. The loop calculates the shifted value and write it to dst,
> > and then immediately reads from src to calculate the carry for the next
> > iteration. Because src and dst could point to the same memory location,
> > the carry is incorrectly calculated using the newly modified dst value
> > instead of the original src value.
> > 
> > Adding a temporary local variable to cache the original value before
> > writing to dst and using it for the carry calculation solves the
> > problem. In addition, partial overlap is rejected from control plane for
> > all kind of operations. This was tested with the following bytecode:
> > 
> > table test_table ip flags 0 use 1 handle 1
> > ip test_table test_chain use 3 type filter hook input prio 0 policy accept packets 0 bytes 0 flags 1
> > ip test_table test_chain 2
> >   [ immediate reg 1 0x44332211 0x88776655 ]
> >   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
> >   [ cmp eq reg 1 0x66443322 0x00887766 ]
> >   [ counter pkts 0 bytes 0 ]
> > ip test_table test_chain 4 3
> >   [ immediate reg 1 0x44332211 0x88776655 ]
> >   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
> >   [ cmp eq reg 1 0x55443322 0x00887766 ]
> >   [ counter pkts 21794 bytes 1917798 ]
> > 
> > Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
> > Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> > ---
> > v2: handled partially register overlap
> > v3: reject partially overlap from control plane
> > v4: applied the partial overlap check to all operations
> > ---
> >  net/netfilter/nft_bitwise.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > index 13808e9cd999..76e7ae96429d 100644
> > --- a/net/netfilter/nft_bitwise.c
> > +++ b/net/netfilter/nft_bitwise.c
[...]
> > @@ -264,6 +269,12 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	n = DIV_ROUND_UP(priv->len, sizeof(u32));
> > +	if (priv->sreg != priv->dreg &&
> > +	    priv->dreg < priv->sreg + n &&
> > +	    priv->sreg < priv->dreg + n)
> > +		return -EINVAL;
> 
> In some cases, there is also sreg2 that probably needs to be handled
> too.

And probably nft_byteorder needs something similar to check for
partial overlaps too for sreg and dreg. Also nft_lookup.

Maybe add this to a helper function and use it from there?

Re: [PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Pablo Neira Ayuso]

On Thu, Apr 30, 2026 at 08:26:32AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 30, 2026 at 08:11:07AM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Apr 27, 2026 at 11:21:18AM +0200, Fernando Fernandez Mancera wrote:
> > > For lshift and rshift, the shift operations are performed in a loop over
> > > 32-bit words. The loop calculates the shifted value and write it to dst,
> > > and then immediately reads from src to calculate the carry for the next
> > > iteration. Because src and dst could point to the same memory location,
> > > the carry is incorrectly calculated using the newly modified dst value
> > > instead of the original src value.
> > > 
> > > Adding a temporary local variable to cache the original value before
> > > writing to dst and using it for the carry calculation solves the
> > > problem. In addition, partial overlap is rejected from control plane for
> > > all kind of operations. This was tested with the following bytecode:
> > > 
> > > table test_table ip flags 0 use 1 handle 1
> > > ip test_table test_chain use 3 type filter hook input prio 0 policy accept packets 0 bytes 0 flags 1
> > > ip test_table test_chain 2
> > >   [ immediate reg 1 0x44332211 0x88776655 ]
> > >   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
> > >   [ cmp eq reg 1 0x66443322 0x00887766 ]
> > >   [ counter pkts 0 bytes 0 ]
> > > ip test_table test_chain 4 3
> > >   [ immediate reg 1 0x44332211 0x88776655 ]
> > >   [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
> > >   [ cmp eq reg 1 0x55443322 0x00887766 ]
> > >   [ counter pkts 21794 bytes 1917798 ]
> > > 
> > > Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
> > > Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> > > ---
> > > v2: handled partially register overlap
> > > v3: reject partially overlap from control plane
> > > v4: applied the partial overlap check to all operations
> > > ---
> > >  net/netfilter/nft_bitwise.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > > index 13808e9cd999..76e7ae96429d 100644
> > > --- a/net/netfilter/nft_bitwise.c
> > > +++ b/net/netfilter/nft_bitwise.c
> [...]
> > > @@ -264,6 +269,12 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
> > >  	if (err < 0)
> > >  		return err;
> > >  
> > > +	n = DIV_ROUND_UP(priv->len, sizeof(u32));
> > > +	if (priv->sreg != priv->dreg &&
> > > +	    priv->dreg < priv->sreg + n &&
> > > +	    priv->sreg < priv->dreg + n)
> > > +		return -EINVAL;
> > 
> > In some cases, there is also sreg2 that probably needs to be handled
> > too.
> 
> And probably nft_byteorder needs something similar to check for
> partial overlaps too for sreg and dreg. Also nft_lookup.

I think it is only nft_byteorder needs this since iteration pattern is
similar.

> Maybe add this to a helper function and use it from there?

Re: [PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> And probably nft_byteorder needs something similar to check for
> partial overlaps too for sreg and dreg. Also nft_lookup.

nft_lookup might be fine.  Key could be larger than result, and vice
versa.  Userspace could be chaining lookups too.  I think we should not
restrict nft_lookup.

Re: [PATCH nf v4] netfilter: nft_bitwise: fix dst corruption in same register shifts

[Pablo Neira Ayuso]

On Thu, Apr 30, 2026 at 08:47:10AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > And probably nft_byteorder needs something similar to check for
> > partial overlaps too for sreg and dreg. Also nft_lookup.
> 
> nft_lookup might be fine.  Key could be larger than result, and vice
> versa.  Userspace could be chaining lookups too.  I think we should not
> restrict nft_lookup.

Agreed.