netlink_delinearize: incorrect meta protocol dependency kill again

This patch adds __meta_dependency_may_kill() to consolidate inspection
of the meta protocol, nfproto and ether type expression to validate
dependency removal on listings.

Phil reports that 567ea4774e13 includes an update on the ip and ip6
families that is not described in the patch, moreover, it flips the
default verdict from true to false.

Fixes: 567ea4774e13 ("netlink_delinearize: incorrect meta protocol dependency kill")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

3 files changed, 151 insertions, 45 deletions

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 92617a46..f2207ea1 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -1983,6 +1983,55 @@ static void payload_match_postprocess(struct rule_pp_ctx *ctx,
 	}
 }
 
+static uint8_t ether_type_to_nfproto(uint16_t l3proto)
+{
+	switch(l3proto) {
+	case ETH_P_IP:
+		return NFPROTO_IPV4;
+	case ETH_P_IPV6:
+		return NFPROTO_IPV6;
+	default:
+		break;
+	}
+
+	return NFPROTO_UNSPEC;
+}
+
+static bool __meta_dependency_may_kill(const struct expr *dep, uint8_t *nfproto)
+{
+	uint16_t l3proto;
+
+	switch (dep->left->etype) {
+	case EXPR_META:
+		switch (dep->left->meta.key) {
+		case NFT_META_NFPROTO:
+			*nfproto = mpz_get_uint8(dep->right->value);
+			break;
+		case NFT_META_PROTOCOL:
+			l3proto = mpz_get_uint16(dep->right->value);
+			*nfproto = ether_type_to_nfproto(l3proto);
+			break;
+		default:
+			return true;
+		}
+		break;
+	case EXPR_PAYLOAD:
+		if (dep->left->payload.base != PROTO_BASE_LL_HDR)
+			return true;
+
+		if (dep->left->dtype != &ethertype_type)
+			return true;
+
+		l3proto = mpz_get_uint16(dep->right->value);
+		*nfproto = ether_type_to_nfproto(l3proto);
+		break;
+	default:
+		return true;
+	}
+
+	return false;
+}
+
 /* We have seen a protocol key expression that restricts matching at the network
  * base, leave it in place since this is meaninful in bridge, inet and netdev
  * families. Exceptions are ICMP and ICMPv6 where this code assumes that can
@@ -1992,67 +2041,34 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
 				     unsigned int family,
 				     const struct expr *expr)
 {
+	uint8_t l4proto, nfproto = NFPROTO_UNSPEC;
 	struct expr *dep = ctx->pdep->expr;
-	uint16_t l3proto, protocol;
-	uint8_t l4proto;
 
 	if (ctx->pbase != PROTO_BASE_NETWORK_HDR)
 		return true;
 
+	if (__meta_dependency_may_kill(dep, &nfproto))
+		return true;
+
 	switch (family) {
 	case NFPROTO_INET:
 	case NFPROTO_NETDEV:
 	case NFPROTO_BRIDGE:
 		break;
 	default:
-		if (dep->left->etype != EXPR_META ||
-		    dep->right->etype != EXPR_VALUE)
+		if (family == NFPROTO_IPV4 &&
+		    nfproto != NFPROTO_IPV4)
+			return false;
+		else if (family == NFPROTO_IPV6 &&
+			 nfproto != NFPROTO_IPV6)
 			return false;
 
-		if (dep->left->meta.key == NFT_META_PROTOCOL) {
-			protocol = mpz_get_uint16(dep->right->value);
-
-			if (family == NFPROTO_IPV4 &&
-			    protocol == ETH_P_IP)
-				return true;
-			else if (family == NFPROTO_IPV6 &&
-				 protocol == ETH_P_IPV6)
-				return true;
-		}
-
-		return false;
+		return true;
 	}
 
 	if (expr->left->meta.key != NFT_META_L4PROTO)
 		return true;
 
-	l3proto = mpz_get_uint16(dep->right->value);
-
-	switch (dep->left->etype) {
-	case EXPR_META:
-		if (dep->left->meta.key != NFT_META_NFPROTO &&
-		    dep->left->meta.key != NFT_META_PROTOCOL)
-			return true;
-		break;
-	case EXPR_PAYLOAD:
-		if (dep->left->payload.base != PROTO_BASE_LL_HDR)
-			return true;
-
-		switch(l3proto) {
-		case ETH_P_IP:
-			l3proto = NFPROTO_IPV4;
-			break;
-		case ETH_P_IPV6:
-			l3proto = NFPROTO_IPV6;
-			break;
-		default:
-			break;
-		}
-		break;
-	default:
-		break;
-	}
-
 	l4proto = mpz_get_uint8(expr->right->value);
 
 	switch (l4proto) {
@@ -2063,8 +2079,8 @@ static bool meta_may_dependency_kill(struct payload_dep_ctx *ctx,
 		return false;
 	}
 
-	if ((l3proto == NFPROTO_IPV4 && l4proto == IPPROTO_ICMPV6) ||
-	    (l3proto == NFPROTO_IPV6 && l4proto == IPPROTO_ICMP))
+	if ((nfproto == NFPROTO_IPV4 && l4proto == IPPROTO_ICMPV6) ||
+	    (nfproto == NFPROTO_IPV6 && l4proto == IPPROTO_ICMP))
 		return false;
 
 	return true;
diff --git a/tests/shell/testcases/optimizations/dependency_kill b/tests/shell/testcases/optimizations/dependency_kill
new file mode 100755
index 00000000..904eecf8
--- /dev/null
+++ b/tests/shell/testcases/optimizations/dependency_kill
@@ -0,0 +1,48 @@
+#!/bin/bash
+
+set -e
+
+RULESET="table bridge foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table ip foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table ip6 foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table netdev foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table inet foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+		meta nfproto ipv4 udp dport 67
+		meta nfproto ipv6 udp dport 67
+	}
+}"
+
+$NFT -f - <<< $RULESET
diff --git a/tests/shell/testcases/optimizations/dumps/dependency_kill.nft b/tests/shell/testcases/optimizations/dumps/dependency_kill.nft
new file mode 100644
index 00000000..1781f7be
--- /dev/null
+++ b/tests/shell/testcases/optimizations/dumps/dependency_kill.nft
@@ -0,0 +1,42 @@
+table bridge foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table ip foo {
+	chain bar {
+		udp dport 67
+		meta protocol ip6 udp dport 67
+		udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table ip6 foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		udp dport 67
+		ether type ip udp dport 67
+		udp dport 67
+	}
+}
+table netdev foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+	}
+}
+table inet foo {
+	chain bar {
+		meta protocol ip udp dport 67
+		meta protocol ip6 udp dport 67
+		ether type ip udp dport 67
+		ether type ip6 udp dport 67
+		meta nfproto ipv4 udp dport 67
+		meta nfproto ipv6 udp dport 67
+	}
+}