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

[PATCH nf v3] 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.
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>
Acked-by: Jeremy Sowden <jeremy@azazel.net>
---
v2: handled partially register overlap
v3: reject partially overlap from control plane, added back Acked-by
from Jeremy Snowden as he provided it for v1
---
 net/netfilter/nft_bitwise.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 13808e9cd999..c1a3e690f4a4 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);
 	}
 }
 
@@ -177,6 +181,7 @@ static int nft_bitwise_init_mask_xor(struct nft_bitwise *priv,
 static int nft_bitwise_init_shift(struct nft_bitwise *priv,
 				  const struct nlattr *const tb[])
 {
+	unsigned int n = DIV_ROUND_UP(priv->len, sizeof(u32));
 	struct nft_data_desc desc = {
 		.type	= NFT_DATA_VALUE,
 		.size	= sizeof(priv->data),
@@ -201,6 +206,11 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
 		return -EINVAL;
 	}
 
+	if (priv->sreg != priv->dreg &&
+	    priv->dreg < priv->sreg + n &&
+	    priv->sreg < priv->dreg + n)
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
2.53.0

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

[Pablo Neira Ayuso]

Hi Fernando,

On Thu, Apr 23, 2026 at 05:54:53PM +0200, Fernando Fernandez Mancera wrote:
[...]
> @@ -201,6 +206,11 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
>  		return -EINVAL;
>  	}
>  
> +	if (priv->sreg != priv->dreg &&
> +	    priv->dreg < priv->sreg + n &&
> +	    priv->sreg < priv->dreg + n)

Is this enough? Just to make sure we are on the same page.

NFT_REG_1
NFT_REG_2
NFT_REG_3
NFT_REG_4

have a size of 128 bytes.

Then, NFT_REG32_00, NFT_REG32_01, NFT_REG32_02 and NFT_REG32_03
basically overlap with NFT_REG_1. They split the 128 bytes of
NFT_REG_1 in 4 registers of 32 bytes.

Is this check above enough to deal with the partial overlaps?

Thanks!

> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.53.0
> 
> 

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

[Fernando Fernandez Mancera]

On 4/24/26 1:18 PM, Pablo Neira Ayuso wrote:
> Hi Fernando,
> 
> On Thu, Apr 23, 2026 at 05:54:53PM +0200, Fernando Fernandez Mancera wrote:
> [...]
>> @@ -201,6 +206,11 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (priv->sreg != priv->dreg &&
>> +	    priv->dreg < priv->sreg + n &&
>> +	    priv->sreg < priv->dreg + n)
> 
> Is this enough? Just to make sure we are on the same page.
> 
> NFT_REG_1
> NFT_REG_2
> NFT_REG_3
> NFT_REG_4
> 
> have a size of 128 bytes.
> 

Right but if I am not wrong these registers are mapped/normalized. That 
happens during nft_parse_register() earlier in the init() path.

Therefore we must expect priv->sreg/dreg in the range [4, 19].

> Then, NFT_REG32_00, NFT_REG32_01, NFT_REG32_02 and NFT_REG32_03
> basically overlap with NFT_REG_1. They split the 128 bytes of
> NFT_REG_1 in 4 registers of 32 bytes.
> 
> Is this check above enough to deal with the partial overlaps?
> 

I am not very good at math but as long as we have the length of the data 
we can calculate the overlap in 4 bytes segments. Of course if from 
userspace you mix both APIs the math should hold up.

let's say we have NFT_REG_1 as sreg and NFT_REG32_O1 as dreg and length 
of 8 bytes.

That is after normalization:

sreg: 4 and dreg: 5

sreg expands through registers 4 and 5
dreg expands through registers 5 and 6

The check is able to catch it. Of course, if the length would be 4 
bytes, the check would pass but that is fine.

At the end NFT_REG_1 is mapped to 32bits register number 4 while 
NFT_REG32_O1 is mapped to 32 bits register number 5.

Does this make sense? Anyway, AI suggested if this should be applied 
XOR, OR, AND, etc. I think yes, as the partial overlap could corrupt the 
result there too. So a v4 is needed anyway.

Thanks,
Fernando.

> Thanks!
> 
>> +		return -EINVAL;
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.53.0
>>
>>
> 

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

[Pablo Neira Ayuso]

Hi Fernando,

On Fri, Apr 24, 2026 at 05:03:17PM +0200, Fernando Fernandez Mancera wrote:
> On 4/24/26 1:18 PM, Pablo Neira Ayuso wrote:
> > Hi Fernando,
> > 
> > On Thu, Apr 23, 2026 at 05:54:53PM +0200, Fernando Fernandez Mancera wrote:
> > [...]
> > > @@ -201,6 +206,11 @@ static int nft_bitwise_init_shift(struct nft_bitwise *priv,
> > >   		return -EINVAL;
> > >   	}
> > > +	if (priv->sreg != priv->dreg &&
> > > +	    priv->dreg < priv->sreg + n &&
> > > +	    priv->sreg < priv->dreg + n)
> > 
> > Is this enough? Just to make sure we are on the same page.
> > 
> > NFT_REG_1
> > NFT_REG_2
> > NFT_REG_3
> > NFT_REG_4
> > 
> > have a size of 128 bytes.
> > 
> 
> Right but if I am not wrong these registers are mapped/normalized. That
> happens during nft_parse_register() earlier in the init() path.

Indeed, registers has been already normalized by nft_parse_register()
at this stage.

> Therefore we must expect priv->sreg/dreg in the range [4, 19].
> 
> > Then, NFT_REG32_00, NFT_REG32_01, NFT_REG32_02 and NFT_REG32_03
> > basically overlap with NFT_REG_1. They split the 128 bytes of
> > NFT_REG_1 in 4 registers of 32 bytes.
> > 
> > Is this check above enough to deal with the partial overlaps?
> > 
> 
> I am not very good at math but as long as we have the length of the data we
> can calculate the overlap in 4 bytes segments. Of course if from userspace
> you mix both APIs the math should hold up.
> 
> let's say we have NFT_REG_1 as sreg and NFT_REG32_O1 as dreg and length of 8
> bytes.
> 
> That is after normalization:
> 
> sreg: 4 and dreg: 5
> 
> sreg expands through registers 4 and 5
> dreg expands through registers 5 and 6
> 
> The check is able to catch it. Of course, if the length would be 4 bytes,
> the check would pass but that is fine.
> 
> At the end NFT_REG_1 is mapped to 32bits register number 4 while
> NFT_REG32_O1 is mapped to 32 bits register number 5.
> 
> Does this make sense? Anyway, AI suggested if this should be applied XOR,
> OR, AND, etc. I think yes, as the partial overlap could corrupt the result
> there too. So a v4 is needed anyway.

OK, let's do that.

Thanks.