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

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

[Fernando Fernandez Mancera]

In nft_bitwise_eval_lshift() and nft_bitwise_eval_rshift(), shift
operations are performed in a loop over 32-bit words. The loop
calculates the shifted value, writes it to dst, and calculates the carry
from src for the next iteration.

If the source and destination registers overlap either exactly (sreg ==
dreg) or partially (e.g., dreg is offset from sreg by 4 bytes) the loop
overwrites src data before it can be read by subsequent iterations. This
causes the carry or the shifted data itself to be incorrectly calculated
using the newly modified dst value instead of the original src payload.

Fix this by separating the calculation and assignment phases. Evaluate
the shift into a temporary local array and then copy the final results
into dst. This completely decouples the memory reads from the memory
writes, safely supporting any register overlap configuration without
corrupting the payload.

This was tested for both partial and exact overlaps 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 0xaaaaaaaa 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 9 << 0x08000000 ) ]
  [ cmp eq reg 1 0x55008877 0x00887766 ]
  [ counter pkts 0 bytes 0 ]
ip test_table test_chain 3 2
  [ immediate reg 1 0xaaaaaaaa 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 9 << 0x08000000 ) ]
  [ cmp eq reg 1 0x55443322 0x00887766 ]
  [ counter pkts 1511 bytes 133128 ]
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 109 bytes 9548 ]
ip test_table test_chain 5 4
  [ immediate reg 1 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
  [ cmp eq reg 1 0x66443322 0x00887766 ]
  [ counter pkts 0 bytes 0 ]

Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
v2: handle partially register overlap too
---
 net/netfilter/nft_bitwise.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 13808e9cd999..27fc09ec0e11 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -38,27 +38,35 @@ static void nft_bitwise_eval_mask_xor(u32 *dst, const u32 *src,
 static void nft_bitwise_eval_lshift(u32 *dst, const u32 *src,
 				    const struct nft_bitwise *priv)
 {
+	unsigned int i, n = DIV_ROUND_UP(priv->len, sizeof(u32));
 	u32 shift = priv->data.data[0];
-	unsigned int i;
+	u32 res[NFT_REG_SIZE];
 	u32 carry = 0;
 
-	for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
-		dst[i - 1] = (src[i - 1] << shift) | carry;
+	for (i = n; i > 0; i--) {
+		res[i - 1] = (src[i - 1] << shift) | carry;
 		carry = src[i - 1] >> (BITS_PER_TYPE(u32) - shift);
 	}
+
+	for (i = 0; i < n; i++)
+		dst[i] = res[i];
 }
 
 static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
 				    const struct nft_bitwise *priv)
 {
+	unsigned int i, n = DIV_ROUND_UP(priv->len, sizeof(u32));
 	u32 shift = priv->data.data[0];
-	unsigned int i;
+	u32 res[NFT_REG_SIZE];
 	u32 carry = 0;
 
-	for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
-		dst[i] = carry | (src[i] >> shift);
+	for (i = 0; i < n; i++) {
+		res[i] = carry | (src[i] >> shift);
 		carry = src[i] << (BITS_PER_TYPE(u32) - shift);
 	}
+
+	for (i = 0; i < n; i++)
+		dst[i] = res[i];
 }
 
 static void nft_bitwise_eval_and(u32 *dst, const u32 *src, const u32 *src2,
-- 
2.53.0

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

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> In nft_bitwise_eval_lshift() and nft_bitwise_eval_rshift(), shift
> operations are performed in a loop over 32-bit words. The loop
> calculates the shifted value, writes it to dst, and calculates the carry
> from src for the next iteration.
> 
> If the source and destination registers overlap either exactly (sreg ==
> dreg) or partially (e.g., dreg is offset from sreg by 4 bytes) the loop
> overwrites src data before it can be read by subsequent iterations. This
> causes the carry or the shifted data itself to be incorrectly calculated
> using the newly modified dst value instead of the original src payload.

We should support overlaps (src == dst).  But partial overlaps?
Does nft generate such byte code?

I think we should reject it.  Either userspace has no more use
for src (and can clobber it with the shifted result), or it has
to keep it for later reuse.  But in what case is a partially clobbered
register useful?

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

[Fernando Fernandez Mancera]

On 4/23/26 2:27 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> In nft_bitwise_eval_lshift() and nft_bitwise_eval_rshift(), shift
>> operations are performed in a loop over 32-bit words. The loop
>> calculates the shifted value, writes it to dst, and calculates the carry
>> from src for the next iteration.
>>
>> If the source and destination registers overlap either exactly (sreg ==
>> dreg) or partially (e.g., dreg is offset from sreg by 4 bytes) the loop
>> overwrites src data before it can be read by subsequent iterations. This
>> causes the carry or the shifted data itself to be incorrectly calculated
>> using the newly modified dst value instead of the original src payload.
> 
> We should support overlaps (src == dst).  But partial overlaps?
> Does nft generate such byte code?
> 

No, not at all.

> I think we should reject it.  Either userspace has no more use
> for src (and can clobber it with the shifted result), or it has
> to keep it for later reuse.  But in what case is a partially clobbered
> register useful?
> 

Fine for me. I could not find any situation where this could be useful 
but I was afraid of breaking some weird setups. If we have agreement 
that this isn't useful I am fine rejecting it from control plane with 
something like (not tested):

unsigned int 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;

Thanks,
Fernando.

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

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 4/23/26 2:27 PM, Florian Westphal wrote:
> > We should support overlaps (src == dst).  But partial overlaps?
> > Does nft generate such byte code?
> > 
> 
> No, not at all.

Phew. :-)

> Fine for me. I could not find any situation where this could be useful 
> but I was afraid of breaking some weird setups. If we have agreement 
> that this isn't useful I am fine rejecting it from control plane with 
> something like (not tested):

Lets wait for Pablos take on this, but I would prefer reject from
control plane.

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

[Pablo Neira Ayuso]

On Thu, Apr 23, 2026 at 02:38:14PM +0200, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> > On 4/23/26 2:27 PM, Florian Westphal wrote:
> > > We should support overlaps (src == dst).  But partial overlaps?
> > > Does nft generate such byte code?
> > > 
> > 
> > No, not at all.
> 
> Phew. :-)
> 
> > Fine for me. I could not find any situation where this could be useful 
> > but I was afraid of breaking some weird setups. If we have agreement 
> > that this isn't useful I am fine rejecting it from control plane with 
> > something like (not tested):
> 
> Lets wait for Pablos take on this, but I would prefer reject from
> control plane.

I'd rather reject this from control plane too, thanks!

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

[Fernando Fernandez Mancera]

On 4/23/26 4:49 PM, Pablo Neira Ayuso wrote:
> On Thu, Apr 23, 2026 at 02:38:14PM +0200, Florian Westphal wrote:
>> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>>> On 4/23/26 2:27 PM, Florian Westphal wrote:
>>>> We should support overlaps (src == dst).  But partial overlaps?
>>>> Does nft generate such byte code?
>>>>
>>>
>>> No, not at all.
>>
>> Phew. :-)
>>
>>> Fine for me. I could not find any situation where this could be useful
>>> but I was afraid of breaking some weird setups. If we have agreement
>>> that this isn't useful I am fine rejecting it from control plane with
>>> something like (not tested):
>>
>> Lets wait for Pablos take on this, but I would prefer reject from
>> control plane.
> 
> I'd rather reject this from control plane too, thanks!

Got it, then I will take the v1 and add the control plane check. Thanks!