| Message ID | 20221130113718.85576-1-fw@strlen.de |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [RFC,ebtables-nft] unify ether type and meta protocol decoding | expand |
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
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.
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
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, ðproto, 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, ðproto, 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; } }
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(-)