diff mbox series

[RFC,nf-next,3/4] netfilter: nf_tables: insert register zeroing instructions for dodgy chains

Message ID 20240627135330.17039-4-fw@strlen.de
State RFC
Headers show
Series nf_tables: remove explicit register zeroing | expand

Commit Message

Florian Westphal June 27, 2024, 1:53 p.m. UTC 
Instead of rejecting rules that read from registers that saw no store,
insert nft_imm instruction preamble when building the ruleset blob.

Once any rule triggers 'uninitied access', table gets marked as
need-rebuild, then all base-chains in the affected table are regenerated.

Known drawback: 'nft monitor trace' may show 'unkown rule handle 0
verdict continue' when this auto-zero is active.
If this is unwanted, the trace infra in kernel could be patched to
suppress notification for handle-0 rules.

As normal rulesets generated by nft or iptables-nft never cause such
uninitialised reads this allows to revert the forced zeroing in the
next patch.

I would not add this patch and keep the reject behaviour, as the
nftables uapi is specifically built around the rule being a standalone
object.  I also question if it makes real sense to do such preload from
userspace, it has little benefit for well-formed (non-repetitive) rulesets.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_tables.h | 10 +++-
 net/netfilter/nf_tables_api.c     | 82 ++++++++++++++++++++++++++++---
 2 files changed, 85 insertions(+), 7 deletions(-)

Comments

Pablo Neira Ayuso July 1, 2024, 8:30 p.m. UTC | #1 
On Thu, Jun 27, 2024 at 03:53:23PM +0200, Florian Westphal wrote:
> Instead of rejecting rules that read from registers that saw no store,
> insert nft_imm instruction preamble when building the ruleset blob.
> 
> Once any rule triggers 'uninitied access', table gets marked as
> need-rebuild, then all base-chains in the affected table are regenerated.
> 
> Known drawback: 'nft monitor trace' may show 'unkown rule handle 0
> verdict continue' when this auto-zero is active.
> If this is unwanted, the trace infra in kernel could be patched to
> suppress notification for handle-0 rules.
> 
> As normal rulesets generated by nft or iptables-nft never cause such
> uninitialised reads this allows to revert the forced zeroing in the
> next patch.
>
> I would not add this patch and keep the reject behaviour, as the
> nftables uapi is specifically built around the rule being a standalone
> object.  I also question if it makes real sense to do such preload from
> userspace, it has little benefit for well-formed (non-repetitive) rulesets.

I am afraid there won't be an easy way to revert this in this future?

Is there any specific concern you have? Buggy validation allowing to
access uninitialized registers? In that case, there is a need to
improve test infrastructure to exercise this code more.

Thanks.
Florian Westphal July 1, 2024, 10:18 p.m. UTC | #2 
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I would not add this patch and keep the reject behaviour, as the
> > nftables uapi is specifically built around the rule being a standalone
> > object.  I also question if it makes real sense to do such preload from
> > userspace, it has little benefit for well-formed (non-repetitive) rulesets.
> 
> I am afraid there won't be an easy way to revert this in this future?
> 
> Is there any specific concern you have? Buggy validation allowing to
> access uninitialized registers? In that case, there is a need to
> improve test infrastructure to exercise this code more.

Yes, for one thing, but I also do not see how we can ever move to a
model where registers are re-used by subsequent rules, its incompatible
with the rule-is-smallest-replaceable-object design.

(Meaning: userspace needs to be fully cooperative and aware that
 it cannot insert a random rule at location x).
Pablo Neira Ayuso July 1, 2024, 10:32 p.m. UTC | #3 
On Tue, Jul 02, 2024 at 12:18:30AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > I would not add this patch and keep the reject behaviour, as the
> > > nftables uapi is specifically built around the rule being a standalone
> > > object.  I also question if it makes real sense to do such preload from
> > > userspace, it has little benefit for well-formed (non-repetitive) rulesets.
> > 
> > I am afraid there won't be an easy way to revert this in this future?
> > 
> > Is there any specific concern you have? Buggy validation allowing to
> > access uninitialized registers? In that case, there is a need to
> > improve test infrastructure to exercise this code more.
> 
> Yes, for one thing, but I also do not see how we can ever move to a
> model where registers are re-used by subsequent rules, its incompatible
> with the rule-is-smallest-replaceable-object design.

Yes, incremental updates are an issue. Another possibility is to add
support for static rulesets, so there is a simple way for userspace to
recycle registers (this would be fully performed from userspace).

And users can still inject raw bytecode to make their own programs. We
have been discussing that dumping a listing with bytecode that cannot
be interpreted is an option to deal with "forward compatibility",
similar approach could help deal with this.

If your concern is the register tracking from the kernel, I am not
pursuing that approach anymore and I can make a patch to ditch it
after this series.

> (Meaning: userspace needs to be fully cooperative and aware that
>  it cannot insert a random rule at location x).
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3f06ec7dc500..f974724a0c90 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -221,6 +221,7 @@  struct nft_ctx {
 	u8				family;
 	u8				level;
 	bool				report;
+	bool				reg_bad;
 	DECLARE_BITMAP(reg_inited, NFT_REG32_NUM);
 };
 
@@ -1247,6 +1248,12 @@  static inline void nft_use_inc_restore(u32 *use)
 
 #define nft_use_dec_restore	nft_use_dec
 
+enum nft_need_reg_zeroing {
+	NFT_REG_ZERO_NO,
+	NFT_REG_ZERO_NEED,
+	NFT_REG_ZERO_ON,
+};
+
 /**
  *	struct nft_table - nf_tables table
  *
@@ -1283,9 +1290,10 @@  struct nft_table {
 					genmask:2;
 	u32				nlpid;
 	char				*name;
-	u16				udlen;
 	u8				*udata;
+	u16				udlen;
 	u8				validate_state;
+	enum nft_need_reg_zeroing	zero_basechains:8;
 };
 
 static inline bool nft_table_has_owner(const struct nft_table *table)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c59a32ab9145..213f3627b5b6 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -146,6 +146,7 @@  static void nft_ctx_init(struct nft_ctx *ctx,
 	ctx->report	= nlmsg_report(nlh);
 	ctx->flags	= nlh->nlmsg_flags;
 	ctx->seq	= nlh->nlmsg_seq;
+	ctx->reg_bad	= false;
 
 	bitmap_zero(ctx->reg_inited, NFT_REG32_NUM);
 }
@@ -543,9 +544,17 @@  static struct nft_trans *nft_trans_rule_add(struct nft_ctx *ctx, int msg_type,
 	if (trans == NULL)
 		return NULL;
 
-	if (msg_type == NFT_MSG_NEWRULE && ctx->nla[NFTA_RULE_ID] != NULL) {
-		nft_trans_rule_id(trans) =
-			ntohl(nla_get_be32(ctx->nla[NFTA_RULE_ID]));
+	if (msg_type == NFT_MSG_NEWRULE) {
+		struct nft_table *table = ctx->table;
+
+		if (ctx->nla[NFTA_RULE_ID]) {
+			nft_trans_rule_id(trans) =
+				ntohl(nla_get_be32(ctx->nla[NFTA_RULE_ID]));
+		}
+
+		if (ctx->reg_bad &&
+		    table->zero_basechains == NFT_REG_ZERO_NO)
+			table->zero_basechains = NFT_REG_ZERO_NEED;
 	}
 	nft_trans_rule(trans) = rule;
 	nft_trans_rule_chain(trans) = ctx->chain;
@@ -9613,11 +9622,58 @@  static bool nft_expr_reduce(struct nft_regs_track *track,
 	return false;
 }
 
+static unsigned int nft_reg_zero_size(void)
+{
+	return offsetof(struct nft_rule_dp, data) +
+	       NFT_EXPR_SIZE(sizeof(struct nft_immediate_expr)) * (NFT_REG_MAX + 1);
+}
+
+static unsigned int nft_reg_zero_add(struct nft_rule_blob *b)
+{
+	static const struct nft_expr_ops imm_zops = {
+		.eval = nft_immediate_eval,
+		.size = NFT_EXPR_SIZE(sizeof(struct nft_immediate_expr)),
+	};
+	struct nft_rule_dp *prule;
+	unsigned int size = 0;
+	void *data = (void *)b->data;
+	int i;
+
+	prule = data;
+	data += offsetof(struct nft_rule_dp, data);
+
+	for (i = 0; i <= NFT_REG_MAX; i++) {
+		struct nft_immediate_expr imm = {
+			.dlen = NFT_REG_SIZE,
+			.dreg = i * NFT_REG32_SIZE,
+		};
+		struct nft_expr *e = data;
+
+		if (i == 0)
+			imm.data.verdict.code = NFT_CONTINUE;
+
+		e->ops = &imm_zops;
+		memcpy(&e->data, &imm, sizeof(imm));
+		data += e->ops->size;
+		size += e->ops->size;
+	}
+
+	prule->is_last = 0;
+	prule->dlen = size;
+	prule->handle = 0;
+
+	size += offsetof(struct nft_rule_dp, data);
+	b->size += size;
+
+	return size;
+}
+
 static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *chain)
 {
 	const struct nft_expr *expr, *last;
 	struct nft_regs_track track = {};
 	unsigned int size, data_size;
+	bool need_register_zeroing;
 	void *data, *data_boundary;
 	struct nft_rule_dp *prule;
 	struct nft_rule *rule;
@@ -9627,6 +9683,10 @@  static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
 		return 0;
 
 	data_size = 0;
+	need_register_zeroing = chain->table->zero_basechains != NFT_REG_ZERO_NO;
+	if (need_register_zeroing)
+		data_size = nft_reg_zero_size();
+
 	list_for_each_entry(rule, &chain->rules, list) {
 		if (nft_is_active_next(net, rule)) {
 			data_size += sizeof(*prule) + rule->dlen;
@@ -9639,9 +9699,18 @@  static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
 	if (!chain->blob_next)
 		return -ENOMEM;
 
+	if (need_register_zeroing)
+		size = nft_reg_zero_add(chain->blob_next);
+	else
+		size = 0;
+
 	data = (void *)chain->blob_next->data;
 	data_boundary = data + data_size;
-	size = 0;
+
+	prule = (struct nft_rule_dp *)data;
+	data += size;
+	if (WARN_ON_ONCE(data > data_boundary))
+		return -ENOMEM;
 
 	list_for_each_entry(rule, &chain->rules, list) {
 		if (!nft_is_active_next(net, rule))
@@ -11104,9 +11173,10 @@  static int nft_validate_register_load(enum nft_registers reg, unsigned int len)
 	return 0;
 }
 
-int nft_parse_register_load(const struct nft_ctx *ctx,
+int nft_parse_register_load(const struct nft_ctx *__ctx,
 			    const struct nlattr *attr, u8 *sreg, u32 len)
 {
+	struct nft_ctx *ctx = (struct nft_ctx *)__ctx;
 	int err, invalid_reg;
 	u32 reg, next_register;
 
@@ -11129,7 +11199,7 @@  int nft_parse_register_load(const struct nft_ctx *ctx,
 
 	/* invalid register within the range that we're loading from? */
 	if (invalid_reg < next_register)
-		return -ENODATA;
+		ctx->reg_bad = true;
 
 	*sreg = reg;
 	return 0;