[PATCH iptables] nft: fix interface comparisons in `-C` commands

[PATCH iptables] nft: fix interface comparisons in `-C` commands

[Jeremy Sowden]

Commit 9ccae6397475 ("nft: Leave interface masks alone when parsing from
kernel") removed code which explicitly set interface masks to all ones.  The
result of this is that they are zero.  However, they are used to mask interfaces
in `is_same_interfaces`.  Consequently, the masked values are alway zero, the
comparisons are always true, and check commands which ought to fail succeed:

  # iptables -N test
  # iptables -A test -i lo \! -o lo -j REJECT
  # iptables -v -L test
  Chain test (0 references)
   pkts bytes target     prot opt in     out     source               destination
      0     0 REJECT     all  --  lo     !lo     anywhere             anywhere             reject-with icmp-port-unreachable
  # iptables -v -C test -i abcdefgh \! -o abcdefgh -j REJECT
  REJECT  all opt -- in lo out !lo  0.0.0.0/0  -> 0.0.0.0/0   reject-with icmp-port-unreachable

Remove the mask parameters from `is_same_interfaces`.  Add a test-case.

Fixes: 9ccae6397475 ("nft: Leave interface masks alone when parsing from kernel")
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-arp.c                            | 10 ++-------
 iptables/nft-ipv4.c                           |  4 +---
 iptables/nft-ipv6.c                           |  6 +-----
 iptables/nft-shared.c                         | 21 +++----------------
 iptables/nft-shared.h                         |  6 +-----
 .../nft-only/0020-compare-interfaces_0        |  9 ++++++++
 6 files changed, 17 insertions(+), 39 deletions(-)
 create mode 100755 iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 264864c3fb2b..c11d64c36863 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -385,14 +385,8 @@ static bool nft_arp_is_same(const struct iptables_command_state *cs_a,
 		return false;
 	}
 
-	return is_same_interfaces(a->arp.iniface,
-				  a->arp.outiface,
-				  (unsigned char *)a->arp.iniface_mask,
-				  (unsigned char *)a->arp.outiface_mask,
-				  b->arp.iniface,
-				  b->arp.outiface,
-				  (unsigned char *)b->arp.iniface_mask,
-				  (unsigned char *)b->arp.outiface_mask);
+	return is_same_interfaces(a->arp.iniface, a->arp.outiface,
+				  b->arp.iniface, b->arp.outiface);
 }
 
 static void nft_arp_save_chain(const struct nftnl_chain *c, const char *policy)
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 740928757b7e..0c8bd2911d10 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -113,9 +113,7 @@ static bool nft_ipv4_is_same(const struct iptables_command_state *a,
 	}
 
 	return is_same_interfaces(a->fw.ip.iniface, a->fw.ip.outiface,
-				  a->fw.ip.iniface_mask, a->fw.ip.outiface_mask,
-				  b->fw.ip.iniface, b->fw.ip.outiface,
-				  b->fw.ip.iniface_mask, b->fw.ip.outiface_mask);
+				  b->fw.ip.iniface, b->fw.ip.outiface);
 }
 
 static void nft_ipv4_set_goto_flag(struct iptables_command_state *cs)
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index b184f8af3e6e..4dbb2af20605 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -99,11 +99,7 @@ static bool nft_ipv6_is_same(const struct iptables_command_state *a,
 	}
 
 	return is_same_interfaces(a->fw6.ipv6.iniface, a->fw6.ipv6.outiface,
-				  a->fw6.ipv6.iniface_mask,
-				  a->fw6.ipv6.outiface_mask,
-				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface,
-				  b->fw6.ipv6.iniface_mask,
-				  b->fw6.ipv6.outiface_mask);
+				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface);
 }
 
 static void nft_ipv6_set_goto_flag(struct iptables_command_state *cs)
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 6775578b1e36..fab77b011963 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -220,31 +220,16 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r,
 }
 
 bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
-			unsigned const char *a_iniface_mask,
-			unsigned const char *a_outiface_mask,
-			const char *b_iniface, const char *b_outiface,
-			unsigned const char *b_iniface_mask,
-			unsigned const char *b_outiface_mask)
+			const char *b_iniface, const char *b_outiface)
 {
 	int i;
 
 	for (i = 0; i < IFNAMSIZ; i++) {
-		if (a_iniface_mask[i] != b_iniface_mask[i]) {
-			DEBUGP("different iniface mask %x, %x (%d)\n",
-			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
-			return false;
-		}
-		if ((a_iniface[i] & a_iniface_mask[i])
-		    != (b_iniface[i] & b_iniface_mask[i])) {
+		if (a_iniface[i] != b_iniface[i]) {
 			DEBUGP("different iniface\n");
 			return false;
 		}
-		if (a_outiface_mask[i] != b_outiface_mask[i]) {
-			DEBUGP("different outiface mask\n");
-			return false;
-		}
-		if ((a_outiface[i] & a_outiface_mask[i])
-		    != (b_outiface[i] & b_outiface_mask[i])) {
+		if (a_outiface[i] != b_outiface[i]) {
 			DEBUGP("different outiface\n");
 			return false;
 		}
diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
index 51d1e4609a3b..b57aee1f84a8 100644
--- a/iptables/nft-shared.h
+++ b/iptables/nft-shared.h
@@ -105,11 +105,7 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r, uint8_t proto, uint
 void add_compat(struct nftnl_rule *r, uint32_t proto, bool inv);
 
 bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
-			unsigned const char *a_iniface_mask,
-			unsigned const char *a_outiface_mask,
-			const char *b_iniface, const char *b_outiface,
-			unsigned const char *b_iniface_mask,
-			unsigned const char *b_outiface_mask);
+			const char *b_iniface, const char *b_outiface);
 
 void __get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, uint8_t *op);
 void get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, bool *inv);
diff --git a/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0 b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
new file mode 100755
index 000000000000..278cd648ebb7
--- /dev/null
+++ b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
@@ -0,0 +1,9 @@
+#!/bin/bash
+
+[[ $XT_MULTI == *xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+
+$XT_MULTI iptables -N test
+$XT_MULTI iptables -A test -i lo \! -o lo -j REJECT
+$XT_MULTI iptables -C test -i abcdefgh \! -o abcdefgh -j REJECT 2>/dev/null && exit 1
+
+exit 0
-- 
2.45.2

Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 6909 bytes --]

On 2024-11-18, at 13:56:50 +0000, Jeremy Sowden wrote:
> Commit 9ccae6397475 ("nft: Leave interface masks alone when parsing from
> kernel") removed code which explicitly set interface masks to all ones.  The
> result of this is that they are zero.  However, they are used to mask interfaces
> in `is_same_interfaces`.  Consequently, the masked values are alway zero, the
> comparisons are always true, and check commands which ought to fail succeed:
> 
>   # iptables -N test
>   # iptables -A test -i lo \! -o lo -j REJECT
>   # iptables -v -L test
>   Chain test (0 references)
>    pkts bytes target     prot opt in     out     source               destination
>       0     0 REJECT     all  --  lo     !lo     anywhere             anywhere             reject-with icmp-port-unreachable
>   # iptables -v -C test -i abcdefgh \! -o abcdefgh -j REJECT
>   REJECT  all opt -- in lo out !lo  0.0.0.0/0  -> 0.0.0.0/0   reject-with icmp-port-unreachable
> 
> Remove the mask parameters from `is_same_interfaces`.  Add a test-case.

Forgot to include a link to the Debian bug report:

Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1087210
> Fixes: 9ccae6397475 ("nft: Leave interface masks alone when parsing from kernel")
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  iptables/nft-arp.c                            | 10 ++-------
>  iptables/nft-ipv4.c                           |  4 +---
>  iptables/nft-ipv6.c                           |  6 +-----
>  iptables/nft-shared.c                         | 21 +++----------------
>  iptables/nft-shared.h                         |  6 +-----
>  .../nft-only/0020-compare-interfaces_0        |  9 ++++++++
>  6 files changed, 17 insertions(+), 39 deletions(-)
>  create mode 100755 iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> 
> diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
> index 264864c3fb2b..c11d64c36863 100644
> --- a/iptables/nft-arp.c
> +++ b/iptables/nft-arp.c
> @@ -385,14 +385,8 @@ static bool nft_arp_is_same(const struct iptables_command_state *cs_a,
>  		return false;
>  	}
>  
> -	return is_same_interfaces(a->arp.iniface,
> -				  a->arp.outiface,
> -				  (unsigned char *)a->arp.iniface_mask,
> -				  (unsigned char *)a->arp.outiface_mask,
> -				  b->arp.iniface,
> -				  b->arp.outiface,
> -				  (unsigned char *)b->arp.iniface_mask,
> -				  (unsigned char *)b->arp.outiface_mask);
> +	return is_same_interfaces(a->arp.iniface, a->arp.outiface,
> +				  b->arp.iniface, b->arp.outiface);
>  }
>  
>  static void nft_arp_save_chain(const struct nftnl_chain *c, const char *policy)
> diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
> index 740928757b7e..0c8bd2911d10 100644
> --- a/iptables/nft-ipv4.c
> +++ b/iptables/nft-ipv4.c
> @@ -113,9 +113,7 @@ static bool nft_ipv4_is_same(const struct iptables_command_state *a,
>  	}
>  
>  	return is_same_interfaces(a->fw.ip.iniface, a->fw.ip.outiface,
> -				  a->fw.ip.iniface_mask, a->fw.ip.outiface_mask,
> -				  b->fw.ip.iniface, b->fw.ip.outiface,
> -				  b->fw.ip.iniface_mask, b->fw.ip.outiface_mask);
> +				  b->fw.ip.iniface, b->fw.ip.outiface);
>  }
>  
>  static void nft_ipv4_set_goto_flag(struct iptables_command_state *cs)
> diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
> index b184f8af3e6e..4dbb2af20605 100644
> --- a/iptables/nft-ipv6.c
> +++ b/iptables/nft-ipv6.c
> @@ -99,11 +99,7 @@ static bool nft_ipv6_is_same(const struct iptables_command_state *a,
>  	}
>  
>  	return is_same_interfaces(a->fw6.ipv6.iniface, a->fw6.ipv6.outiface,
> -				  a->fw6.ipv6.iniface_mask,
> -				  a->fw6.ipv6.outiface_mask,
> -				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface,
> -				  b->fw6.ipv6.iniface_mask,
> -				  b->fw6.ipv6.outiface_mask);
> +				  b->fw6.ipv6.iniface, b->fw6.ipv6.outiface);
>  }
>  
>  static void nft_ipv6_set_goto_flag(struct iptables_command_state *cs)
> diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> index 6775578b1e36..fab77b011963 100644
> --- a/iptables/nft-shared.c
> +++ b/iptables/nft-shared.c
> @@ -220,31 +220,16 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r,
>  }
>  
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask)
> +			const char *b_iniface, const char *b_outiface)
>  {
>  	int i;
>  
>  	for (i = 0; i < IFNAMSIZ; i++) {
> -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> -			DEBUGP("different iniface mask %x, %x (%d)\n",
> -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> -			return false;
> -		}
> -		if ((a_iniface[i] & a_iniface_mask[i])
> -		    != (b_iniface[i] & b_iniface_mask[i])) {
> +		if (a_iniface[i] != b_iniface[i]) {
>  			DEBUGP("different iniface\n");
>  			return false;
>  		}
> -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> -			DEBUGP("different outiface mask\n");
> -			return false;
> -		}
> -		if ((a_outiface[i] & a_outiface_mask[i])
> -		    != (b_outiface[i] & b_outiface_mask[i])) {
> +		if (a_outiface[i] != b_outiface[i]) {
>  			DEBUGP("different outiface\n");
>  			return false;
>  		}
> diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> index 51d1e4609a3b..b57aee1f84a8 100644
> --- a/iptables/nft-shared.h
> +++ b/iptables/nft-shared.h
> @@ -105,11 +105,7 @@ void add_l4proto(struct nft_handle *h, struct nftnl_rule *r, uint8_t proto, uint
>  void add_compat(struct nftnl_rule *r, uint32_t proto, bool inv);
>  
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask);
> +			const char *b_iniface, const char *b_outiface);
>  
>  void __get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, uint8_t *op);
>  void get_cmp_data(struct nftnl_expr *e, void *data, size_t dlen, bool *inv);
> diff --git a/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0 b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> new file mode 100755
> index 000000000000..278cd648ebb7
> --- /dev/null
> +++ b/iptables/tests/shell/testcases/nft-only/0020-compare-interfaces_0
> @@ -0,0 +1,9 @@
> +#!/bin/bash
> +
> +[[ $XT_MULTI == *xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
> +
> +$XT_MULTI iptables -N test
> +$XT_MULTI iptables -A test -i lo \! -o lo -j REJECT
> +$XT_MULTI iptables -C test -i abcdefgh \! -o abcdefgh -j REJECT 2>/dev/null && exit 1
> +
> +exit 0
> -- 
> 2.45.2
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Eric Garver]

On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
> Commit 9ccae6397475 ("nft: Leave interface masks alone when parsing from
> kernel") removed code which explicitly set interface masks to all ones.  The
> result of this is that they are zero.  However, they are used to mask interfaces
> in `is_same_interfaces`.  Consequently, the masked values are alway zero, the
> comparisons are always true, and check commands which ought to fail succeed:
> 
>   # iptables -N test
>   # iptables -A test -i lo \! -o lo -j REJECT
>   # iptables -v -L test
>   Chain test (0 references)
>    pkts bytes target     prot opt in     out     source               destination
>       0     0 REJECT     all  --  lo     !lo     anywhere             anywhere             reject-with icmp-port-unreachable
>   # iptables -v -C test -i abcdefgh \! -o abcdefgh -j REJECT
>   REJECT  all opt -- in lo out !lo  0.0.0.0/0  -> 0.0.0.0/0   reject-with icmp-port-unreachable
> 
> Remove the mask parameters from `is_same_interfaces`.  Add a test-case.
> 
> Fixes: 9ccae6397475 ("nft: Leave interface masks alone when parsing from kernel")
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>

This patch also fixes a rule ordering regression (same Fixes) that Phil
and I discovered last week. Thank you!

Tested-by: Eric Garver <eric@garver.life>

Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Phil Sutter]

Hi Jeremy,

On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
[...]
> Remove the mask parameters from `is_same_interfaces`.  Add a test-case.

Thanks for the fix and test-case!

Some remarks below:

[...]
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask)
> +			const char *b_iniface, const char *b_outiface)
>  {
>  	int i;
>  
>  	for (i = 0; i < IFNAMSIZ; i++) {
> -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> -			DEBUGP("different iniface mask %x, %x (%d)\n",
> -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> -			return false;
> -		}
> -		if ((a_iniface[i] & a_iniface_mask[i])
> -		    != (b_iniface[i] & b_iniface_mask[i])) {
> +		if (a_iniface[i] != b_iniface[i]) {
>  			DEBUGP("different iniface\n");
>  			return false;
>  		}
> -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> -			DEBUGP("different outiface mask\n");
> -			return false;
> -		}
> -		if ((a_outiface[i] & a_outiface_mask[i])
> -		    != (b_outiface[i] & b_outiface_mask[i])) {
> +		if (a_outiface[i] != b_outiface[i]) {
>  			DEBUGP("different outiface\n");
>  			return false;
>  		}

My draft fix converts this to strncmp() calls, I don't think we should
inspect bytes past the NUL-char. Usually we parse into a zeroed
iptables_command_state, but if_indextoname(3P) does not define output
buffer contents apart from "shall place in this buffer the name of the
interface", so it may put garbage in there (although unlikely).

Another thing is a potential follow-up: There are remains in
nft_arp_post_parse() and ipv6_post_parse(), needless filling of the mask
buffers. They may be dropped along with the now unused mask fields in
struct xtables_args.

WDYT?

Thanks, Phil

Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]

On 2024-11-19, at 14:20:31 +0100, Phil Sutter wrote:
> On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
> > Remove the mask parameters from `is_same_interfaces`.  Add a test-case.
> 
> Thanks for the fix and test-case!
> 
> Some remarks below:
> 
> [...]
> >  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> > -			unsigned const char *a_iniface_mask,
> > -			unsigned const char *a_outiface_mask,
> > -			const char *b_iniface, const char *b_outiface,
> > -			unsigned const char *b_iniface_mask,
> > -			unsigned const char *b_outiface_mask)
> > +			const char *b_iniface, const char *b_outiface)
> >  {
> >  	int i;
> >  
> >  	for (i = 0; i < IFNAMSIZ; i++) {
> > -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> > -			DEBUGP("different iniface mask %x, %x (%d)\n",
> > -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> > -			return false;
> > -		}
> > -		if ((a_iniface[i] & a_iniface_mask[i])
> > -		    != (b_iniface[i] & b_iniface_mask[i])) {
> > +		if (a_iniface[i] != b_iniface[i]) {
> >  			DEBUGP("different iniface\n");
> >  			return false;
> >  		}
> > -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> > -			DEBUGP("different outiface mask\n");
> > -			return false;
> > -		}
> > -		if ((a_outiface[i] & a_outiface_mask[i])
> > -		    != (b_outiface[i] & b_outiface_mask[i])) {
> > +		if (a_outiface[i] != b_outiface[i]) {
> >  			DEBUGP("different outiface\n");
> >  			return false;
> >  		}
> 
> My draft fix converts this to strncmp() calls, I don't think we should
> inspect bytes past the NUL-char. Usually we parse into a zeroed
> iptables_command_state, but if_indextoname(3P) does not define output
> buffer contents apart from "shall place in this buffer the name of the
> interface", so it may put garbage in there (although unlikely).

Seems reasonable.  I was so focussed on the masks and bit-twiddling that
I lost sight of the fact that the code is looping to compare strings. :)

> Another thing is a potential follow-up: There are remains in
> nft_arp_post_parse() and ipv6_post_parse(), needless filling of the mask
> buffers. They may be dropped along with the now unused mask fields in
> struct xtables_args.

Yes, I spotted those.  I couldn't see how they were used, but I was
reasonably sure that they weren't related to this bug, so I stopped
looking.

> WDYT?

Agreed on both counts.  Shall I incorporate your suggestions and send a
v2 or do you have something already prepared?

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Phil Sutter]

On Tue, Nov 19, 2024 at 08:07:00PM +0000, Jeremy Sowden wrote:
> On 2024-11-19, at 14:20:31 +0100, Phil Sutter wrote:
> > On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
> > > Remove the mask parameters from `is_same_interfaces`.  Add a test-case.
> > 
> > Thanks for the fix and test-case!
> > 
> > Some remarks below:
> > 
> > [...]
> > >  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> > > -			unsigned const char *a_iniface_mask,
> > > -			unsigned const char *a_outiface_mask,
> > > -			const char *b_iniface, const char *b_outiface,
> > > -			unsigned const char *b_iniface_mask,
> > > -			unsigned const char *b_outiface_mask)
> > > +			const char *b_iniface, const char *b_outiface)
> > >  {
> > >  	int i;
> > >  
> > >  	for (i = 0; i < IFNAMSIZ; i++) {
> > > -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> > > -			DEBUGP("different iniface mask %x, %x (%d)\n",
> > > -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> > > -			return false;
> > > -		}
> > > -		if ((a_iniface[i] & a_iniface_mask[i])
> > > -		    != (b_iniface[i] & b_iniface_mask[i])) {
> > > +		if (a_iniface[i] != b_iniface[i]) {
> > >  			DEBUGP("different iniface\n");
> > >  			return false;
> > >  		}
> > > -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> > > -			DEBUGP("different outiface mask\n");
> > > -			return false;
> > > -		}
> > > -		if ((a_outiface[i] & a_outiface_mask[i])
> > > -		    != (b_outiface[i] & b_outiface_mask[i])) {
> > > +		if (a_outiface[i] != b_outiface[i]) {
> > >  			DEBUGP("different outiface\n");
> > >  			return false;
> > >  		}
> > 
> > My draft fix converts this to strncmp() calls, I don't think we should
> > inspect bytes past the NUL-char. Usually we parse into a zeroed
> > iptables_command_state, but if_indextoname(3P) does not define output
> > buffer contents apart from "shall place in this buffer the name of the
> > interface", so it may put garbage in there (although unlikely).
> 
> Seems reasonable.  I was so focussed on the masks and bit-twiddling that
> I lost sight of the fact that the code is looping to compare strings. :)
> 
> > Another thing is a potential follow-up: There are remains in
> > nft_arp_post_parse() and ipv6_post_parse(), needless filling of the mask
> > buffers. They may be dropped along with the now unused mask fields in
> > struct xtables_args.
> 
> Yes, I spotted those.  I couldn't see how they were used, but I was
> reasonably sure that they weren't related to this bug, so I stopped
> looking.
> 
> > WDYT?
> 
> Agreed on both counts.  Shall I incorporate your suggestions and send a
> v2 or do you have something already prepared?

Sent a v2, please review.

Thanks, Phil