[RFC ebtables-nft] unify ether type and meta protocol decoding

[RFC ebtables-nft] unify ether type and meta protocol decoding

[Florian Westphal]

Handle "ether protocol" and "meta protcol" the same.

Problem is that this breaks the test case *again*:

I: [EXECUTING]   iptables/tests/shell/testcases/ebtables/0006-flush_0
--A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
--A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
+-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
+-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP

... because ebtables-nft will now render meta protocol as "-p IPv4".

ebtables-legacy does not have any special handling for this.

Solving this would need more internal annotations during decode, so
we can suppress/ignore "meta protocol" once a "among-type" set is
encountered.

Any (other) suggestions?

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 iptables/nft-bridge.c | 74 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 13 deletions(-)

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 50e90b22cf2f..4488ff172c2e 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -188,6 +188,64 @@ static int nft_bridge_add(struct nft_handle *h,
 	return _add_action(r, cs);
 }
 
+static bool nft_bridge_parse_ethproto(struct nft_xt_ctx *ctx,
+				      struct nftnl_expr *e,
+				      struct iptables_command_state *cs)
+{
+	struct ebt_entry *fw = &cs->eb;
+	bool already_seen;
+	uint16_t ethproto;
+	uint8_t op;
+
+	already_seen = (fw->bitmask & EBT_NOPROTO) == 0;
+
+	__get_cmp_data(e, &ethproto, sizeof(ethproto), &op);
+
+	switch (op) {
+	case NFT_CMP_EQ:
+		if (already_seen && fw->invflags & EBT_IPROTO) {
+			ctx->errmsg = "ethproto eq test contradicts previous";
+			return false;
+		}
+		break;
+	case NFT_CMP_NEQ:
+		if (already_seen && (fw->invflags & EBT_IPROTO) == 0) {
+			ctx->errmsg = "ethproto ne test contradicts previous";
+			return false;
+		}
+		fw->invflags |= EBT_IPROTO;
+		break;
+	case NFT_CMP_GTE:
+		if (already_seen && (fw->invflags & EBT_IPROTO) == 0) {
+			ctx->errmsg = "ethproto gte test contradicts previous";
+			return false;
+		}
+		fw->invflags |= EBT_IPROTO;
+		/* fallthrough */
+	case NFT_CMP_LT:
+		/* -p Length mode */
+		if (ethproto == htons(0x0600))
+			fw->bitmask |= EBT_802_3;
+		break;
+	default:
+		ctx->errmsg = "ethproto only supports eq/ne test";
+		return false;
+	}
+
+	if (already_seen) {
+		if (fw->ethproto != ethproto) {
+			ctx->errmsg = "ethproto ne test contradicts previous";
+			return false;
+		}
+	} else if ((fw->bitmask & EBT_802_3) == 0) {
+		fw->ethproto = ethproto;
+	}
+
+	fw->bitmask &= ~EBT_NOPROTO;
+
+	return true;
+}
+
 static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
 				  const struct nft_xt_ctx_reg *reg,
 				  struct nftnl_expr *e,
@@ -199,6 +257,7 @@ static void nft_bridge_parse_meta(struct nft_xt_ctx *ctx,
 
 	switch (reg->meta_dreg.key) {
 	case NFT_META_PROTOCOL:
+		nft_bridge_parse_ethproto(ctx, e, cs);
 		return;
 	}
 
@@ -241,8 +300,6 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
 {
 	struct ebt_entry *fw = &cs->eb;
 	unsigned char addr[ETH_ALEN];
-	unsigned short int ethproto;
-	uint8_t op;
 	bool inv;
 	int i;
 
@@ -275,17 +332,8 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
 		fw->bitmask |= EBT_ISOURCE;
 		break;
 	case offsetof(struct ethhdr, h_proto):
-		__get_cmp_data(e, &ethproto, sizeof(ethproto), &op);
-		if (ethproto == htons(0x0600)) {
-			fw->bitmask |= EBT_802_3;
-			inv = (op == NFT_CMP_GTE);
-		} else {
-			fw->ethproto = ethproto;
-			inv = (op == NFT_CMP_NEQ);
-		}
-		if (inv)
-			fw->invflags |= EBT_IPROTO;
-		fw->bitmask &= ~EBT_NOPROTO;
+		if (!nft_bridge_parse_ethproto(ctx, e, cs))
+			return;
 		break;
 	}
 }
-- 
2.38.1

Re: [RFC ebtables-nft] unify ether type and meta protocol decoding

[Phil Sutter]

On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote:
> Handle "ether protocol" and "meta protcol" the same.
> 
> Problem is that this breaks the test case *again*:
> 
> I: [EXECUTING]   iptables/tests/shell/testcases/ebtables/0006-flush_0
> --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> 
> ... because ebtables-nft will now render meta protocol as "-p IPv4".
> 
> ebtables-legacy does not have any special handling for this.
> 
> Solving this would need more internal annotations during decode, so
> we can suppress/ignore "meta protocol" once a "among-type" set is
> encountered.
> 
> Any (other) suggestions?

Since ebtables among does not support IPv6, match elimination should be
pretty simple, no? Entirely untested:

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 08c93feeba2c9..0daebfd983127 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
        if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt))
                xtables_error(OTHER_PROBLEM,
                              "ebtables among pair parsing failed");
+
+       if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) &&
+           ctx->cs.eb.ethproto == htons(0x0800))
+               ctx->cs.eb.bitmask |= EBT_NOPROTO;
 }
 
 static void parse_watcher(void *object, struct ebt_match **match_list,

Cheers, Phil

Re: [RFC ebtables-nft] unify ether type and meta protocol decoding

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote:
> > Handle "ether protocol" and "meta protcol" the same.
> > 
> > Problem is that this breaks the test case *again*:
> > 
> > I: [EXECUTING]   iptables/tests/shell/testcases/ebtables/0006-flush_0
> > --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> > --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> > +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> > +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> > 
> > ... because ebtables-nft will now render meta protocol as "-p IPv4".
> > 
> > ebtables-legacy does not have any special handling for this.
> > 
> > Solving this would need more internal annotations during decode, so
> > we can suppress/ignore "meta protocol" once a "among-type" set is
> > encountered.
> > 
> > Any (other) suggestions?
> 
> Since ebtables among does not support IPv6, match elimination should be
> pretty simple, no? Entirely untested:
> 
> diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> index 08c93feeba2c9..0daebfd983127 100644
> --- a/iptables/nft-bridge.c
> +++ b/iptables/nft-bridge.c
> @@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
>         if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt))
>                 xtables_error(OTHER_PROBLEM,
>                               "ebtables among pair parsing failed");
> +
> +       if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) &&
> +           ctx->cs.eb.ethproto == htons(0x0800))
> +               ctx->cs.eb.bitmask |= EBT_NOPROTO;

But that would munge
ebtables-nft -p ipv4 ....
ebtables-nft ....

We don't know if "-p" was added explicitly or not.

Re: [RFC ebtables-nft] unify ether type and meta protocol decoding

[Phil Sutter]

On Thu, Dec 01, 2022 at 11:16:03AM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Wed, Nov 30, 2022 at 12:37:18PM +0100, Florian Westphal wrote:
> > > Handle "ether protocol" and "meta protcol" the same.
> > > 
> > > Problem is that this breaks the test case *again*:
> > > 
> > > I: [EXECUTING]   iptables/tests/shell/testcases/ebtables/0006-flush_0
> > > --A FORWARD --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> > > --A OUTPUT --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> > > +-A FORWARD -p IPv4 --among-dst fe:ed:ba:be:13:37=10.0.0.1 -j ACCEPT
> > > +-A OUTPUT -p IPv4 --among-src c0:ff:ee:90:0:0=192.168.0.1 -j DROP
> > > 
> > > ... because ebtables-nft will now render meta protocol as "-p IPv4".
> > > 
> > > ebtables-legacy does not have any special handling for this.
> > > 
> > > Solving this would need more internal annotations during decode, so
> > > we can suppress/ignore "meta protocol" once a "among-type" set is
> > > encountered.
> > > 
> > > Any (other) suggestions?
> > 
> > Since ebtables among does not support IPv6, match elimination should be
> > pretty simple, no? Entirely untested:
> > 
> > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> > index 08c93feeba2c9..0daebfd983127 100644
> > --- a/iptables/nft-bridge.c
> > +++ b/iptables/nft-bridge.c
> > @@ -520,6 +520,10 @@ static void nft_bridge_parse_lookup(struct nft_xt_ctx *ctx,
> >         if (set_elems_to_among_pairs(among_data->pairs + poff, s, cnt))
> >                 xtables_error(OTHER_PROBLEM,
> >                               "ebtables among pair parsing failed");
> > +
> > +       if (!(ctx->cs.eb.bitmask & EBT_NOPROTO) &&
> > +           ctx->cs.eb.ethproto == htons(0x0800))
> > +               ctx->cs.eb.bitmask |= EBT_NOPROTO;
> 
> But that would munge
> ebtables-nft -p ipv4 ....
> ebtables-nft ....
> 
> We don't know if "-p" was added explicitly or not.

Ah, the infamous explicit vs. implicit problem. :(

Looking at ebt_among.c in kernel, it seems packets which are neither
IPv4 nor ARP are treated as non-matching. Since ebtables-nft doesn't
support ARP with among anyway, can we assume people will not specify '-p
ipv4' when using among?

Cheers, Phil

[iptables RFC] ebtables: among: Embed meta protocol match into set lookup

[Phil Sutter]

This way the match will be consumed by among match parser and not
confused with explicit '-p IPv4' match.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
This is a bit of a hack to avoid the implicit vs. explicit confusion but
it works and is backwards compatible. WDYT?
---
 extensions/libebt_among.c |  1 +
 iptables/nft-bridge.c     | 65 ++++++++++++++++++++++++---------------
 iptables/nft-bridge.h     |  1 +
 iptables/nft.c            | 30 ++++++++++++------
 4 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/extensions/libebt_among.c b/extensions/libebt_among.c
index a80fb80404ee1..49580ea6d3fc2 100644
--- a/extensions/libebt_among.c
+++ b/extensions/libebt_among.c
@@ -69,6 +69,7 @@ parse_nft_among_pair(char *buf, struct nft_among_pair *pair, bool have_ip)
 		if (!inet_pton(AF_INET, sep + 1, &pair->in))
 			xtables_error(PARAMETER_PROBLEM,
 				      "Invalid IP address '%s'", sep + 1);
+		pair->proto = htons(ETH_P_IP);
 	}
 	ether = ether_aton(buf);
 	if (!ether)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index e223d19765f90..ebfc5de50b236 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -335,17 +335,26 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx,
 	const struct nft_xt_ctx_reg *reg;
 	int val, val2 = -1;
 
-	reg = nft_xt_ctx_get_sreg(ctx, sreg);
-	if (!reg)
-		return -1;
-
-	if (reg->type != NFT_XT_REG_PAYLOAD) {
-		ctx->errmsg = "lookup reg is not payload type";
+	switch (key_len) {
+	case 6:		/* ether */
+	case 12:	/* + ipv4addr */
+	case 16:	/* + meta protocol */
+		break;
+	default:
+		ctx->errmsg = "unsupported lookup key length";
 		return -1;
 	}
 
-	switch (key_len) {
-	case 12: /* ether + ipv4addr */
+	if (key_len >= 6) { /* ether */
+		reg = nft_xt_ctx_get_sreg(ctx, sreg);
+		if (!reg)
+			return -1;
+
+		if (reg->type != NFT_XT_REG_PAYLOAD) {
+			ctx->errmsg = "lookup reg is not payload type";
+			return -1;
+		}
+
 		val = lookup_check_ether_payload(reg->payload.base,
 						 reg->payload.offset,
 						 reg->payload.len);
@@ -355,7 +364,8 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx,
 			       reg->payload.len);
 			return -1;
 		}
-
+	}
+	if (key_len >= 12) { /* + ipv4addr */
 		sreg = nft_get_next_reg(sreg, ETH_ALEN);
 
 		reg = nft_xt_ctx_get_sreg(ctx, sreg);
@@ -381,23 +391,25 @@ static int lookup_analyze_payloads(struct nft_xt_ctx *ctx,
 			DEBUGP("mismatching payload match offsets\n");
 			return -1;
 		}
-		break;
-	case 6: /* ether */
-		val = lookup_check_ether_payload(reg->payload.base,
-						 reg->payload.offset,
-						 reg->payload.len);
-		if (val < 0) {
-			DEBUGP("unknown payload base/offset/len %d/%d/%d\n",
-			       reg->payload.base, reg->payload.offset,
-			       reg->payload.len);
+	}
+	if (key_len == 16) {
+		sreg = nft_get_next_reg(sreg, sizeof(struct in_addr));
+		reg = nft_xt_ctx_get_sreg(ctx, sreg);
+		if (!reg) {
+			ctx->errmsg = "next lookup register is invalid";
+			return -1;
+		}
+
+		if (reg->type != NFT_XT_REG_META_DREG) {
+			ctx->errmsg = "next lookup reg is not meta type";
 			return -1;
 		}
-		break;
-	default:
-		ctx->errmsg = "unsupported lookup key length";
-		return -1;
-	}
 
+		if (reg->meta_dreg.key != NFT_META_PROTOCOL) {
+			ctx->errmsg = "meta reg not protocol type";
+			return -1;
+		}
+	}
 	if (dst)
 		*dst = (val == 1);
 	if (ip)
@@ -422,15 +434,18 @@ static int set_elems_to_among_pairs(struct nft_among_pair *pairs,
 
 	while ((elem = nftnl_set_elems_iter_next(iter))) {
 		data = nftnl_set_elem_get(elem, NFTNL_SET_ELEM_KEY, &datalen);
+		struct nft_among_pair pair = {};
+
 		if (!data) {
 			fprintf(stderr, "BUG: set elem without key\n");
 			goto err;
 		}
-		if (datalen > sizeof(*pairs)) {
+		if (datalen > sizeof(pair)) {
 			fprintf(stderr, "BUG: overlong set elem\n");
 			goto err;
 		}
-		nft_among_insert_pair(pairs, &tmpcnt, data);
+		memcpy(&pair, data, datalen);
+		nft_among_insert_pair(pairs, &tmpcnt, &pair);
 	}
 	ret = 0;
 err:
diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
index eb1b3928b6543..c87065ef48195 100644
--- a/iptables/nft-bridge.h
+++ b/iptables/nft-bridge.h
@@ -125,6 +125,7 @@ int ebt_command_default(struct iptables_command_state *cs);
 struct nft_among_pair {
 	struct ether_addr ether;
 	struct in_addr in __attribute__((aligned (4)));
+	uint16_t proto __attribute__((aligned (4)));
 };
 
 struct nft_among_data {
diff --git a/iptables/nft.c b/iptables/nft.c
index 430888e864a5f..05422c91f3827 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1139,6 +1139,7 @@ gen_lookup(uint32_t sreg, const char *set_name, uint32_t set_id, uint32_t flags)
 /* from nftables:include/datatype.h, enum datatypes */
 #define NFT_DATATYPE_IPADDR	7
 #define NFT_DATATYPE_ETHERADDR	9
+#define NFT_DATATYPE_ETHERTYPE	10
 
 static int __add_nft_among(struct nft_handle *h, const char *table,
 			   struct nftnl_rule *r, struct nft_among_pair *pairs,
@@ -1164,6 +1165,11 @@ static int __add_nft_among(struct nft_handle *h, const char *table,
 		type = type << CONCAT_TYPE_BITS | NFT_DATATYPE_IPADDR;
 		len += sizeof(struct in_addr) + NETLINK_ALIGN - 1;
 		len &= ~(NETLINK_ALIGN - 1);
+
+		type = type << CONCAT_TYPE_BITS | NFT_DATATYPE_ETHERTYPE;
+		len += sizeof(uint16_t) + NETLINK_ALIGN - 1;
+		len &= ~(NETLINK_ALIGN - 1);
+
 		flags = NFT_SET_INTERVAL | NFT_SET_CONCAT;
 	}
 
@@ -1173,7 +1179,11 @@ static int __add_nft_among(struct nft_handle *h, const char *table,
 	set_id = nftnl_set_get_u32(s, NFTNL_SET_ID);
 
 	if (ip) {
-		uint8_t field_len[2] = { ETH_ALEN, sizeof(struct in_addr) };
+		uint8_t field_len[] = {
+			ETH_ALEN,
+			sizeof(struct in_addr),
+			sizeof(uint16_t),
+		};
 
 		nftnl_set_set_data(s, NFTNL_SET_DESC_CONCAT,
 				   field_len, sizeof(field_len));
@@ -1211,6 +1221,15 @@ static int __add_nft_among(struct nft_handle *h, const char *table,
 		if (!e)
 			return -ENOMEM;
 		nftnl_rule_add_expr(r, e);
+
+		/* FIXME: Fix add_meta() instead to accept a reg */
+		reg = nft_get_next_reg(reg, sizeof(struct in_addr));
+		e = nftnl_expr_alloc("meta");
+		if (!e)
+			return -ENOMEM;
+		nftnl_expr_set_u32(e, NFTNL_EXPR_META_KEY, NFT_META_PROTOCOL);
+		nftnl_expr_set_u32(e, NFTNL_EXPR_META_DREG, reg);
+		nftnl_rule_add_expr(r, e);
 	}
 
 	reg = NFT_REG_1;
@@ -1228,15 +1247,6 @@ static int add_nft_among(struct nft_handle *h,
 	struct nft_among_data *data = (struct nft_among_data *)m->data;
 	const char *table = nftnl_rule_get(r, NFTNL_RULE_TABLE);
 
-	if ((data->src.cnt && data->src.ip) ||
-	    (data->dst.cnt && data->dst.ip)) {
-		uint16_t eth_p_ip = htons(ETH_P_IP);
-		uint8_t reg;
-
-		add_meta(h, r, NFT_META_PROTOCOL, &reg);
-		add_cmp_ptr(r, NFT_CMP_EQ, &eth_p_ip, 2, reg);
-	}
-
 	if (data->src.cnt)
 		__add_nft_among(h, table, r, data->pairs, data->src.cnt,
 				false, data->src.inv, data->src.ip);
-- 
2.38.0