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

[Florian Westphal <fw@strlen.de>]

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(-)

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;
-- 
2.44.2