[PATCH nf-next 2/3] netfilter: nf_tables: validate register loads never access unitialised registers

[Florian Westphal <fw@xxxxxxxxx>]

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@xxxxxxxxx>
---
 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 37 ++++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

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, &reg);
@@ -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,
-- 
2.39.2