| Message ID | 20220919201254.32253-1-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables-nft] iptables-nft: must withdraw PAYLOAD flag after parsing | expand |
Florian Westphal <fw@strlen.de> wrote: > else, next payload is stacked via 'CTX_PREV_PAYLOAD'. > > Example breakage: > > ip saddr 1.2.3.4 meta l4proto tcp > ... is dumped as > -s 6.0.0.0 -p tcp > > iptables-nft -s 1.2.3.4 -p tcp is dumped correctly, because > the expressions are ordered like: > meta l4proto tcp ip saddr 1.2.3.4 > > ... and 'meta l4proto' will clear the PAYLOAD flag. > > Fixes: 250dce876d92 ("nft-shared: support native tcp port delinearize") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > iptables/nft-shared.c | 2 ++ > .../ipt-restore/0018-multi-payload_0 | 27 +++++++++++++++++++ > 2 files changed, 29 insertions(+) > create mode 100755 iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 > > diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c > index 71e2f18dab92..66e09e8fd533 100644 > --- a/iptables/nft-shared.c > +++ b/iptables/nft-shared.c > @@ -986,6 +986,8 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e) > nft_parse_transport(ctx, e, ctx->cs); > break; > } > + > + ctx->flags &= ~NFT_XT_CTX_PAYLOAD; > } This isn't ideal either since this breaks dissection of '1-42' ranges that use two compare operands, i.e.: cmp reg1 gte 1 cmp reg1 lte 42 ...as first cmp 'hides' reg1 again. I'd propose to rework this context stuff: no more payload/meta/whatever flags, instead 'mirror' the raw data registers. Other ideas/suggestions?
On Mon, Sep 19, 2022 at 11:31:09PM +0200, Florian Westphal wrote: > Florian Westphal <fw@strlen.de> wrote: > > else, next payload is stacked via 'CTX_PREV_PAYLOAD'. > > > > Example breakage: > > > > ip saddr 1.2.3.4 meta l4proto tcp > > ... is dumped as > > -s 6.0.0.0 -p tcp > > > > iptables-nft -s 1.2.3.4 -p tcp is dumped correctly, because > > the expressions are ordered like: > > meta l4proto tcp ip saddr 1.2.3.4 > > > > ... and 'meta l4proto' will clear the PAYLOAD flag. > > > > Fixes: 250dce876d92 ("nft-shared: support native tcp port delinearize") > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > iptables/nft-shared.c | 2 ++ > > .../ipt-restore/0018-multi-payload_0 | 27 +++++++++++++++++++ > > 2 files changed, 29 insertions(+) > > create mode 100755 iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 > > > > diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c > > index 71e2f18dab92..66e09e8fd533 100644 > > --- a/iptables/nft-shared.c > > +++ b/iptables/nft-shared.c > > @@ -986,6 +986,8 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e) > > nft_parse_transport(ctx, e, ctx->cs); > > break; > > } > > + > > + ctx->flags &= ~NFT_XT_CTX_PAYLOAD; > > } > > This isn't ideal either since this breaks dissection of '1-42' ranges > that use two compare operands, i.e.: > > cmp reg1 gte 1 > cmp reg1 lte 42 > > ...as first cmp 'hides' reg1 again. > > I'd propose to rework this context stuff: > no more payload/meta/whatever flags, instead 'mirror' the raw data > registers. > > Other ideas/suggestions? When do we use multiple flags? I see we need NFT_XT_CTX_BITWISE in addition to NFT_XT_CTX_META. Do we need e.g. NFT_XT_CTX_META and NFT_XT_CTX_PAYLOAD at the same time? My idea would be to use an enum for the LHS expression which is overwritten by each consecutive LHS expression and a bitfield for the "on top" stuff. Cheers, Phil
Phil Sutter <phil@nwl.cc> wrote: > On Mon, Sep 19, 2022 at 11:31:09PM +0200, Florian Westphal wrote: > > Florian Westphal <fw@strlen.de> wrote: > > > else, next payload is stacked via 'CTX_PREV_PAYLOAD'. > > > > > > Example breakage: > > > > > > ip saddr 1.2.3.4 meta l4proto tcp > > > ... is dumped as > > > -s 6.0.0.0 -p tcp > > > > > > iptables-nft -s 1.2.3.4 -p tcp is dumped correctly, because > > > the expressions are ordered like: > > > meta l4proto tcp ip saddr 1.2.3.4 > > > > > > ... and 'meta l4proto' will clear the PAYLOAD flag. > > > > > > Fixes: 250dce876d92 ("nft-shared: support native tcp port delinearize") > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > > --- > > > iptables/nft-shared.c | 2 ++ > > > .../ipt-restore/0018-multi-payload_0 | 27 +++++++++++++++++++ > > > 2 files changed, 29 insertions(+) > > > create mode 100755 iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 > > > > > > diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c > > > index 71e2f18dab92..66e09e8fd533 100644 > > > --- a/iptables/nft-shared.c > > > +++ b/iptables/nft-shared.c > > > @@ -986,6 +986,8 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e) > > > nft_parse_transport(ctx, e, ctx->cs); > > > break; > > > } > > > + > > > + ctx->flags &= ~NFT_XT_CTX_PAYLOAD; > > > } > > > > This isn't ideal either since this breaks dissection of '1-42' ranges > > that use two compare operands, i.e.: > > > > cmp reg1 gte 1 > > cmp reg1 lte 42 > > > > ...as first cmp 'hides' reg1 again. > > > > I'd propose to rework this context stuff: > > no more payload/meta/whatever flags, instead 'mirror' the raw data > > registers. > > > > Other ideas/suggestions? > > When do we use multiple flags? I see we need NFT_XT_CTX_BITWISE in > addition to NFT_XT_CTX_META. Do we need e.g. NFT_XT_CTX_META and > NFT_XT_CTX_PAYLOAD at the same time? No, that makes no sense. I'm working on a new decoder that handles each dreg individually. Then, only one of meta/immediate/payload can be active per register. bitwise can be set in addition. I hope to have a rough RFC draft ready by tomorrw. One big advantage is that we would no longer have to clear the flags in the individual nft_expr -> xt_match/target dissectors. Instead, those are always auto-cleared when a nft expressio writes to the register. > the LHS expression which is overwritten by each consecutive LHS > expression and a bitfield for the "on top" stuff. Yes, thats the same concept that I'm aiming for: +enum nft_ctx_reg_type { + NFT_XT_REG_UNDEF, + NFT_XT_REG_PAYLOAD, + NFT_XT_REG_IMMEDIATE, + NFT_XT_REG_META, +}; + +struct nft_xt_ctx_reg { + enum nft_ctx_reg_type type:8; + + union { + struct { + uint32_t base; + uint32_t offset; + uint32_t len; + } payload; + struct { + uint32_t data[4]; + uint8_t len; + } immediate; + struct { + uint32_t key; + } meta; + }; + + struct { + uint32_t mask[4]; + uint32_t xor[4]; + bool set; + } bitwise; +};
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c index 71e2f18dab92..66e09e8fd533 100644 --- a/iptables/nft-shared.c +++ b/iptables/nft-shared.c @@ -986,6 +986,8 @@ static void nft_parse_cmp(struct nft_xt_ctx *ctx, struct nftnl_expr *e) nft_parse_transport(ctx, e, ctx->cs); break; } + + ctx->flags &= ~NFT_XT_CTX_PAYLOAD; } } diff --git a/iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 b/iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 new file mode 100755 index 000000000000..f27577540d6e --- /dev/null +++ b/iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0 @@ -0,0 +1,27 @@ +#!/bin/bash + +# Make sure iptables-restore simply ignores +# rules starting with -6 + +set -e + +# show rules, drop uninteresting policy settings +ipt_show() { + $XT_MULTI iptables-save | grep -- '-A INPUT' +} + +# issue reproducer for iptables-restore + +$XT_MULTI iptables-restore <<EOF +*filter +-A INPUT -s 1.2.3.0/25 -p udp +-A INPUT -s 1.2.3.0/26 -d 5.6.7.8/32 +-A INPUT -s 1.2.3.0/27 -d 10.2.0.0/16 -p tcp -j ACCEPT +COMMIT +EOF + +EXPECT='-A INPUT -s 1.2.3.0/25 -p udp +-A INPUT -s 1.2.3.0/26 -d 5.6.7.8/32 +-A INPUT -s 1.2.3.0/27 -d 10.2.0.0/16 -p tcp -j ACCEPT' + +diff -u -Z <(echo -e "$EXPECT") <(ipt_show)
else, next payload is stacked via 'CTX_PREV_PAYLOAD'. Example breakage: ip saddr 1.2.3.4 meta l4proto tcp ... is dumped as -s 6.0.0.0 -p tcp iptables-nft -s 1.2.3.4 -p tcp is dumped correctly, because the expressions are ordered like: meta l4proto tcp ip saddr 1.2.3.4 ... and 'meta l4proto' will clear the PAYLOAD flag. Fixes: 250dce876d92 ("nft-shared: support native tcp port delinearize") Signed-off-by: Florian Westphal <fw@strlen.de> --- iptables/nft-shared.c | 2 ++ .../ipt-restore/0018-multi-payload_0 | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100755 iptables/tests/shell/testcases/ipt-restore/0018-multi-payload_0