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@xxxxxxxxx>
---
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
