| Message ID | 20230505111656.32238-3-fw@strlen.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: nf_tables: reject loads from uninitialized registers | expand |
Hi Florian, On Fri, May 05, 2023 at 01:16:55PM +0200, Florian Westphal wrote: > Reject rules where a load occurs from a register that has not seen a > store early in the same rule. > > commit 4c905f6740a3 ("netfilter: nf_tables: initialize registers in nft_do_chain()") > had to add a unconditional memset to the nftables register space to > avoid leaking stack information to userspace. > > This memset shows up in benchmarks. After this change, this commit > can be reverted again. > > Note that this breaks userspace compatibility, because theoretically > you can do > > rule 1: reg2 := meta load iif, reg2 == 1 jump ... > rule 2: reg2 == 2 jump ... // read access with no store in this rule > > ... after this change this is rejected. We can probably achieve the same effect by recovering the runtime register tracking patches I posted. It should be possible to add unlikely() to the branch that checks for uninitialized data in source register, that is missing in this patch: https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-6-pablo@netfilter.org/ such patch also equires these patches to add the helpers to load and store: https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-3-pablo@netfilter.org/ https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-4-pablo@netfilter.org/ https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-5-pablo@netfilter.org/ I think those helpers to load and store on registers should have been there since the beginning instead of opencoding operations on the registers. I am going to collect some numbers with these patches including unlikely() to the _load() checks. I fear I might have introduced some subtle bug, I remember these patches are passing selftests fine, but I am not sure we have enough of these selftests. As you suggested, I also considered using the new track infrastructure (the one I posted to achieve the combo expressions) to detect a read to uninitialized registers from control plane, but it gets complicated again because: 1) what level should register tracking happen at? rule, chain or from basechain to leaf chains (to ensure that we retain the freedom to make more transformation from userspace, eg. static flag for ruleset that never change to omit redundant operations in the generated bytecode). Your patch selects rule level. Chain level will lose context when jumping/going to another chain. Inspecting from basechain to leaf chains will be expensive in dynamic rulesets. 2) combo expressions omit the register store operation, the tracking infrastructure would need to distinguish between two situations: register data has been omitted or register data is missing because userspace provides bytecode that tries to read uninitialized registers. While I agree control plane is ideal for this, because it allows to reject a ruleset that reads on uninitialized register data, checking at rule/chain level cripples expressiveness in a way that it will not be easy to revert in the future if we want to change direction. > Neither nftables nor iptables-nft generate such rules, each rule is > always standalone. That is true these days indeed. I like your approach because it is simple. But my concern is that this limits expressiveness. Thanks.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > rule 1: reg2 := meta load iif, reg2 == 1 jump ... > > rule 2: reg2 == 2 jump ... // read access with no store in this rule > > > > ... after this change this is rejected. > > We can probably achieve the same effect by recovering the runtime > register tracking patches I posted. It should be possible to add > unlikely() to the branch that checks for uninitialized data in source > register, that is missing in this patch: > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-6-pablo@netfilter.org/ > > such patch also equires these patches to add the helpers to load and > store: > > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-3-pablo@netfilter.org/ > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-4-pablo@netfilter.org/ > https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-5-pablo@netfilter.org/ > > I think those helpers to load and store on registers should have been > there since the beginning instead of opencoding operations on the > registers. I am going to collect some numbers with these patches > including unlikely() to the _load() checks. I fear I might have > introduced some subtle bug, I remember these patches are passing > selftests fine, but I am not sure we have enough of these selftests. Thanks, I agree that we will need runtime checking because control plane is too complex due to stores becoming unreliable (NFT_BREAK encountered) if we want to do this load suppression. I get the impression that we're overthinking this, we really should not bother too much with speeding up iptables-style linear rulesets. I'm only concerned with speeding up compact rulesets that use sets/maps wherever possible. I think for those getting rid of the memset() will help more than eliding a couple of reloads. > 1) what level should register tracking happen at? rule, chain or from > basechain to leaf chains (to ensure that we retain the freedom to > make more transformation from userspace, eg. static flag for ruleset > that never change to omit redundant operations in the generated > bytecode). Your patch selects rule level. Chain level will lose > context when jumping/going to another chain. Inspecting from > basechain to leaf chains will be expensive in dynamic rulesets. Right. I used rule level because its easy to do but as you say, it prevents userspace from ever making a "clever" ruleset that performs any sort of preload (without kernel update). > 2) combo expressions omit the register store operation, the tracking > infrastructure would need to distinguish between two situations: > register data has been omitted or register data is missing because > userspace provides bytecode that tries to read uninitialized > registers. > > While I agree control plane is ideal for this, because it allows to > reject a ruleset that reads on uninitialized register data, checking > at rule/chain level cripples expressiveness in a way that it will not > be easy to revert in the future if we want to change direction. > > > Neither nftables nor iptables-nft generate such rules, each rule is > > always standalone. > > That is true these days indeed. I like your approach because it is > simple. But my concern is that this limits expressiveness. Lets wait for your test with runtime checking, so we have some data on how much of a slowdown that gives. But I'd like to see some proof that a *good* ruleset has many redundant loads where such cross-rule load elimination can add a benefit in the first place. Else doing runtime validation makes no sense. What I can think of is to allow this patch in, i.e. rule level enforcement, and then follow a approach similar to what you already proposed earlier: A per-chain prefetch stage. This would mean instead of chain [ rule [ expr, expr, expr ]] [ rule [ expr , expr ]] .. we'd have chain prefetch [ expr, expr ] [ rule ... ] The prefetch is limited to selected meta and payload operations. Failure means entire chain is bypassed. Any sort of jump invalidates the prefetch. So, this patch would be later relaxed to pre-init the "prefetch" registers as "initialised", so following is legal: prefetch: reg3: meta l4roto, reg4: meta load iif rule 1: reg2 := ip saddr, lookup(reg2 . reg3 .reg 4) accept rule 2: reg3 == icmp accept ... // no longer rejected as uninitialized rule 3: reg4 == "eth*" jump ... rule 4: reg3 == icmp accept ... // fails --> prefetch lost rule 4: reg2 := ip saddr ... // fails --> prefetch overwritten Yet another idea: Introduce internal shadow registers: Prefetch to reg1, reg2, reg3 will auto-store those *also* to *pfr1*, *pfr2, and so on. This would allow register content recovery after each jump: reg1 = pfr1 reg2 = pfr2 and so on. But again, I think this is the wrong approach and we should not bother with load elimination or anything else that will complicate the data path. Combo match approach is good because it doesn't have that issue.
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index 246c4a4620ae..8c6068569fcf 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -220,6 +220,7 @@ struct nft_ctx { u8 family; u8 level; bool report; + DECLARE_BITMAP(reg_inited, NFT_REG32_COUNT); }; enum nft_data_desc_flags { diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 4fd6bbb88cd2..cd9deeccda0f 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -139,6 +139,8 @@ static void nft_ctx_init(struct nft_ctx *ctx, ctx->report = nlmsg_report(nlh); ctx->flags = nlh->nlmsg_flags; ctx->seq = nlh->nlmsg_seq; + + memset(ctx->reg_inited, 0, sizeof(ctx->reg_inited)); } static struct nft_trans *nft_trans_alloc_gfp(const struct nft_ctx *ctx, @@ -10039,7 +10041,7 @@ static int nft_validate_register_load(enum nft_registers reg, unsigned int len) int nft_parse_register_load(const struct nft_ctx *ctx, const struct nlattr *attr, u8 *sreg, u32 len) { - u32 reg; + u32 reg, bit; int err; err = nft_parse_register(attr, ®); @@ -10050,11 +10052,37 @@ int nft_parse_register_load(const struct nft_ctx *ctx, if (err < 0) return err; + for (bit = reg; len > 0; bit++) { + if (!test_bit(bit, ctx->reg_inited)) + return -ENODATA; + if (len <= NFT_REG32_SIZE) + break; + len -= NFT_REG32_SIZE; + } + *sreg = reg; return 0; } EXPORT_SYMBOL_GPL(nft_parse_register_load); +static void nft_saw_register_store(const struct nft_ctx *__ctx, + int reg, unsigned int len) +{ + unsigned int last_reg = reg + (len - 1) / NFT_REG32_SIZE; + struct nft_ctx *ctx = (struct nft_ctx *)__ctx; + int bit; + + if (WARN_ON_ONCE(len == 0 || reg < 0)) + return; + + for (bit = reg; bit <= last_reg; bit++) { + if (WARN_ON_ONCE(bit >= NFT_REG32_COUNT)) + return; + + set_bit(bit, ctx->reg_inited); + } +} + static int nft_validate_register_store(const struct nft_ctx *ctx, enum nft_registers reg, const struct nft_data *data, @@ -10076,7 +10104,7 @@ static int nft_validate_register_store(const struct nft_ctx *ctx, return err; } - return 0; + break; default: if (reg < NFT_REG_1 * NFT_REG_SIZE / NFT_REG32_SIZE) return -EINVAL; @@ -10088,8 +10116,11 @@ static int nft_validate_register_store(const struct nft_ctx *ctx, if (data != NULL && type != NFT_DATA_VALUE) return -EINVAL; - return 0; + break; } + + nft_saw_register_store(ctx, reg, len); + return 0; } int nft_parse_register_store(const struct nft_ctx *ctx,
Reject rules where a load occurs from a register that has not seen a store early in the same rule. commit 4c905f6740a3 ("netfilter: nf_tables: initialize registers in nft_do_chain()") had to add a unconditional memset to the nftables register space to avoid leaking stack information to userspace. This memset shows up in benchmarks. After this change, this commit can be reverted again. Note that this breaks userspace compatibility, because theoretically you can do rule 1: reg2 := meta load iif, reg2 == 1 jump ... rule 2: reg2 == 2 jump ... // read access with no store in this rule ... after this change this is rejected. Neither nftables nor iptables-nft generate such rules, each rule is always standalone. Signed-off-by: Florian Westphal <fw@strlen.de> --- include/net/netfilter/nf_tables.h | 1 + net/netfilter/nf_tables_api.c | 37 ++++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-)