[RFC nf-next 0/4] nf_tables: remove explicit register zeroing

[RFC nf-next 0/4] nf_tables: remove explicit register zeroing

[Florian Westphal]

Hi,

I'd like to propose this again.

First patch is preparation work.
Second patch is the actual change I'd like to get into nf-next.

The third patch partially un-does the second:
Instead of rejecting a rule that triggers uninitialised register
access detection, do explicit zeroing from blob generator.

Please see patch 3 for a rationale why I think that we should
just go with patch 1+2.

Patch 4 reverts the explicit zeroing.

Florian Westphal (4):
  netfilter: nf_tables: pass context structure to
    nft_parse_register_load
  netfilter: nf_tables: allow loads only when register is initialized
  netfilter: nf_tables: insert register zeroing instructions for dodgy
    chains
  netfilter: nf_tables: don't initialize registers in nft_do_chain()

 include/net/netfilter/nf_tables.h      |  14 ++-
 net/bridge/netfilter/nft_meta_bridge.c |   2 +-
 net/ipv4/netfilter/nft_dup_ipv4.c      |   4 +-
 net/ipv6/netfilter/nft_dup_ipv6.c      |   4 +-
 net/netfilter/nf_tables_api.c          | 119 +++++++++++++++++++++++--
 net/netfilter/nf_tables_core.c         |   2 +-
 net/netfilter/nft_bitwise.c            |   4 +-
 net/netfilter/nft_byteorder.c          |   2 +-
 net/netfilter/nft_cmp.c                |   6 +-
 net/netfilter/nft_ct.c                 |   2 +-
 net/netfilter/nft_dup_netdev.c         |   2 +-
 net/netfilter/nft_dynset.c             |   4 +-
 net/netfilter/nft_exthdr.c             |   2 +-
 net/netfilter/nft_fwd_netdev.c         |   6 +-
 net/netfilter/nft_hash.c               |   2 +-
 net/netfilter/nft_lookup.c             |   2 +-
 net/netfilter/nft_masq.c               |   4 +-
 net/netfilter/nft_meta.c               |   2 +-
 net/netfilter/nft_nat.c                |   8 +-
 net/netfilter/nft_objref.c             |   2 +-
 net/netfilter/nft_payload.c            |   2 +-
 net/netfilter/nft_queue.c              |   2 +-
 net/netfilter/nft_range.c              |   2 +-
 net/netfilter/nft_redir.c              |   4 +-
 net/netfilter/nft_tproxy.c             |   4 +-
 25 files changed, 159 insertions(+), 48 deletions(-)

-- 
2.44.2

[RFC nf-next 1/4] netfilter: nf_tables: pass context structure to nft_parse_register_load

[Florian Westphal]

Mechanical transformation, no logical changes intended.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_tables.h      | 3 ++-
 net/bridge/netfilter/nft_meta_bridge.c | 2 +-
 net/ipv4/netfilter/nft_dup_ipv4.c      | 4 ++--
 net/ipv6/netfilter/nft_dup_ipv6.c      | 4 ++--
 net/netfilter/nf_tables_api.c          | 3 ++-
 net/netfilter/nft_bitwise.c            | 4 ++--
 net/netfilter/nft_byteorder.c          | 2 +-
 net/netfilter/nft_cmp.c                | 6 +++---
 net/netfilter/nft_ct.c                 | 2 +-
 net/netfilter/nft_dup_netdev.c         | 2 +-
 net/netfilter/nft_dynset.c             | 4 ++--
 net/netfilter/nft_exthdr.c             | 2 +-
 net/netfilter/nft_fwd_netdev.c         | 6 +++---
 net/netfilter/nft_hash.c               | 2 +-
 net/netfilter/nft_lookup.c             | 2 +-
 net/netfilter/nft_masq.c               | 4 ++--
 net/netfilter/nft_meta.c               | 2 +-
 net/netfilter/nft_nat.c                | 8 ++++----
 net/netfilter/nft_objref.c             | 2 +-
 net/netfilter/nft_payload.c            | 2 +-
 net/netfilter/nft_queue.c              | 2 +-
 net/netfilter/nft_range.c              | 2 +-
 net/netfilter/nft_redir.c              | 4 ++--
 net/netfilter/nft_tproxy.c             | 4 ++--
 24 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 1e8da1b882ac..9a71fc20598b 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -254,7 +254,8 @@ static inline enum nft_registers nft_type_to_reg(enum nft_data_types type)
 int nft_parse_u32_check(const struct nlattr *attr, int max, u32 *dest);
 int nft_dump_register(struct sk_buff *skb, unsigned int attr, unsigned int reg);
 
-int nft_parse_register_load(const struct nlattr *attr, u8 *sreg, u32 len);
+int nft_parse_register_load(const struct nft_ctx *ctx,
+			    const struct nlattr *attr, u8 *sreg, u32 len);
 int nft_parse_register_store(const struct nft_ctx *ctx,
 			     const struct nlattr *attr, u8 *dreg,
 			     const struct nft_data *data,
diff --git a/net/bridge/netfilter/nft_meta_bridge.c b/net/bridge/netfilter/nft_meta_bridge.c
index bd4d1b4d745f..4d8e15927217 100644
--- a/net/bridge/netfilter/nft_meta_bridge.c
+++ b/net/bridge/netfilter/nft_meta_bridge.c
@@ -142,7 +142,7 @@ static int nft_meta_bridge_set_init(const struct nft_ctx *ctx,
 	}
 
 	priv->len = len;
-	err = nft_parse_register_load(tb[NFTA_META_SREG], &priv->sreg, len);
+	err = nft_parse_register_load(ctx, tb[NFTA_META_SREG], &priv->sreg, len);
 	if (err < 0)
 		return err;
 
diff --git a/net/ipv4/netfilter/nft_dup_ipv4.c b/net/ipv4/netfilter/nft_dup_ipv4.c
index a522c3a3be52..ef5dd88107dd 100644
--- a/net/ipv4/netfilter/nft_dup_ipv4.c
+++ b/net/ipv4/netfilter/nft_dup_ipv4.c
@@ -40,13 +40,13 @@ static int nft_dup_ipv4_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_DUP_SREG_ADDR] == NULL)
 		return -EINVAL;
 
-	err = nft_parse_register_load(tb[NFTA_DUP_SREG_ADDR], &priv->sreg_addr,
+	err = nft_parse_register_load(ctx, tb[NFTA_DUP_SREG_ADDR], &priv->sreg_addr,
 				      sizeof(struct in_addr));
 	if (err < 0)
 		return err;
 
 	if (tb[NFTA_DUP_SREG_DEV])
-		err = nft_parse_register_load(tb[NFTA_DUP_SREG_DEV],
+		err = nft_parse_register_load(ctx, tb[NFTA_DUP_SREG_DEV],
 					      &priv->sreg_dev, sizeof(int));
 
 	return err;
diff --git a/net/ipv6/netfilter/nft_dup_ipv6.c b/net/ipv6/netfilter/nft_dup_ipv6.c
index c82f3fdd4a65..492a811828a7 100644
--- a/net/ipv6/netfilter/nft_dup_ipv6.c
+++ b/net/ipv6/netfilter/nft_dup_ipv6.c
@@ -38,13 +38,13 @@ static int nft_dup_ipv6_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_DUP_SREG_ADDR] == NULL)
 		return -EINVAL;
 
-	err = nft_parse_register_load(tb[NFTA_DUP_SREG_ADDR], &priv->sreg_addr,
+	err = nft_parse_register_load(ctx, tb[NFTA_DUP_SREG_ADDR], &priv->sreg_addr,
 				      sizeof(struct in6_addr));
 	if (err < 0)
 		return err;
 
 	if (tb[NFTA_DUP_SREG_DEV])
-		err = nft_parse_register_load(tb[NFTA_DUP_SREG_DEV],
+		err = nft_parse_register_load(ctx, tb[NFTA_DUP_SREG_DEV],
 					      &priv->sreg_dev, sizeof(int));
 
 	return err;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 02d75aefaa8e..7437b38269a5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -11102,7 +11102,8 @@ static int nft_validate_register_load(enum nft_registers reg, unsigned int len)
 	return 0;
 }
 
-int nft_parse_register_load(const struct nlattr *attr, u8 *sreg, u32 len)
+int nft_parse_register_load(const struct nft_ctx *ctx,
+			    const struct nlattr *attr, u8 *sreg, u32 len)
 {
 	u32 reg;
 	int err;
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index ca857afbf061..7de95674fd8c 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -171,7 +171,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
 
 	priv->len = len;
 
-	err = nft_parse_register_load(tb[NFTA_BITWISE_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_BITWISE_SREG], &priv->sreg,
 				      priv->len);
 	if (err < 0)
 		return err;
@@ -365,7 +365,7 @@ static int nft_bitwise_fast_init(const struct nft_ctx *ctx,
 	struct nft_bitwise_fast_expr *priv = nft_expr_priv(expr);
 	int err;
 
-	err = nft_parse_register_load(tb[NFTA_BITWISE_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_BITWISE_SREG], &priv->sreg,
 				      sizeof(u32));
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
index f6e791a68101..2f82a444d21b 100644
--- a/net/netfilter/nft_byteorder.c
+++ b/net/netfilter/nft_byteorder.c
@@ -139,7 +139,7 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
 
 	priv->len = len;
 
-	err = nft_parse_register_load(tb[NFTA_BYTEORDER_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_BYTEORDER_SREG], &priv->sreg,
 				      priv->len);
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_cmp.c b/net/netfilter/nft_cmp.c
index cd4652259095..2605f43737bc 100644
--- a/net/netfilter/nft_cmp.c
+++ b/net/netfilter/nft_cmp.c
@@ -83,7 +83,7 @@ static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	if (err < 0)
 		return err;
 
-	err = nft_parse_register_load(tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
+	err = nft_parse_register_load(ctx, tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
 	if (err < 0)
 		return err;
 
@@ -222,7 +222,7 @@ static int nft_cmp_fast_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
-	err = nft_parse_register_load(tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
+	err = nft_parse_register_load(ctx, tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
 	if (err < 0)
 		return err;
 
@@ -323,7 +323,7 @@ static int nft_cmp16_fast_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
-	err = nft_parse_register_load(tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
+	err = nft_parse_register_load(ctx, tb[NFTA_CMP_SREG], &priv->sreg, desc.len);
 	if (err < 0)
 		return err;
 
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 452ed94c3a4d..67a41cd2baaf 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -606,7 +606,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	}
 
 	priv->len = len;
-	err = nft_parse_register_load(tb[NFTA_CT_SREG], &priv->sreg, len);
+	err = nft_parse_register_load(ctx, tb[NFTA_CT_SREG], &priv->sreg, len);
 	if (err < 0)
 		goto err1;
 
diff --git a/net/netfilter/nft_dup_netdev.c b/net/netfilter/nft_dup_netdev.c
index e5739a59ebf1..0573f96ce079 100644
--- a/net/netfilter/nft_dup_netdev.c
+++ b/net/netfilter/nft_dup_netdev.c
@@ -40,7 +40,7 @@ static int nft_dup_netdev_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_DUP_SREG_DEV] == NULL)
 		return -EINVAL;
 
-	return nft_parse_register_load(tb[NFTA_DUP_SREG_DEV], &priv->sreg_dev,
+	return nft_parse_register_load(ctx, tb[NFTA_DUP_SREG_DEV], &priv->sreg_dev,
 				       sizeof(int));
 }
 
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index b4ada3ab2167..6920df754265 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -215,7 +215,7 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 			return err;
 	}
 
-	err = nft_parse_register_load(tb[NFTA_DYNSET_SREG_KEY], &priv->sreg_key,
+	err = nft_parse_register_load(ctx, tb[NFTA_DYNSET_SREG_KEY], &priv->sreg_key,
 				      set->klen);
 	if (err < 0)
 		return err;
@@ -226,7 +226,7 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 		if (set->dtype == NFT_DATA_VERDICT)
 			return -EOPNOTSUPP;
 
-		err = nft_parse_register_load(tb[NFTA_DYNSET_SREG_DATA],
+		err = nft_parse_register_load(ctx, tb[NFTA_DYNSET_SREG_DATA],
 					      &priv->sreg_data, set->dlen);
 		if (err < 0)
 			return err;
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index 6eb571d0c3fd..6bfd33516241 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -588,7 +588,7 @@ static int nft_exthdr_tcp_set_init(const struct nft_ctx *ctx,
 	priv->flags  = flags;
 	priv->op     = op;
 
-	return nft_parse_register_load(tb[NFTA_EXTHDR_SREG], &priv->sreg,
+	return nft_parse_register_load(ctx, tb[NFTA_EXTHDR_SREG], &priv->sreg,
 				       priv->len);
 }
 
diff --git a/net/netfilter/nft_fwd_netdev.c b/net/netfilter/nft_fwd_netdev.c
index 358e742afad7..c83a794025f9 100644
--- a/net/netfilter/nft_fwd_netdev.c
+++ b/net/netfilter/nft_fwd_netdev.c
@@ -52,7 +52,7 @@ static int nft_fwd_netdev_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_FWD_SREG_DEV] == NULL)
 		return -EINVAL;
 
-	return nft_parse_register_load(tb[NFTA_FWD_SREG_DEV], &priv->sreg_dev,
+	return nft_parse_register_load(ctx, tb[NFTA_FWD_SREG_DEV], &priv->sreg_dev,
 				       sizeof(int));
 }
 
@@ -178,12 +178,12 @@ static int nft_fwd_neigh_init(const struct nft_ctx *ctx,
 		return -EOPNOTSUPP;
 	}
 
-	err = nft_parse_register_load(tb[NFTA_FWD_SREG_DEV], &priv->sreg_dev,
+	err = nft_parse_register_load(ctx, tb[NFTA_FWD_SREG_DEV], &priv->sreg_dev,
 				      sizeof(int));
 	if (err < 0)
 		return err;
 
-	return nft_parse_register_load(tb[NFTA_FWD_SREG_ADDR], &priv->sreg_addr,
+	return nft_parse_register_load(ctx, tb[NFTA_FWD_SREG_ADDR], &priv->sreg_addr,
 				       addr_len);
 }
 
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index 868d68302d22..5d034bbb6913 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -92,7 +92,7 @@ static int nft_jhash_init(const struct nft_ctx *ctx,
 
 	priv->len = len;
 
-	err = nft_parse_register_load(tb[NFTA_HASH_SREG], &priv->sreg, len);
+	err = nft_parse_register_load(ctx, tb[NFTA_HASH_SREG], &priv->sreg, len);
 	if (err < 0)
 		return err;
 
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index b314ca728a29..146bf9444c03 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -113,7 +113,7 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
 	if (IS_ERR(set))
 		return PTR_ERR(set);
 
-	err = nft_parse_register_load(tb[NFTA_LOOKUP_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_LOOKUP_SREG], &priv->sreg,
 				      set->klen);
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_masq.c b/net/netfilter/nft_masq.c
index 8a14aaca93bb..cb43c72a8c2a 100644
--- a/net/netfilter/nft_masq.c
+++ b/net/netfilter/nft_masq.c
@@ -52,13 +52,13 @@ static int nft_masq_init(const struct nft_ctx *ctx,
 		priv->flags = ntohl(nla_get_be32(tb[NFTA_MASQ_FLAGS]));
 
 	if (tb[NFTA_MASQ_REG_PROTO_MIN]) {
-		err = nft_parse_register_load(tb[NFTA_MASQ_REG_PROTO_MIN],
+		err = nft_parse_register_load(ctx, tb[NFTA_MASQ_REG_PROTO_MIN],
 					      &priv->sreg_proto_min, plen);
 		if (err < 0)
 			return err;
 
 		if (tb[NFTA_MASQ_REG_PROTO_MAX]) {
-			err = nft_parse_register_load(tb[NFTA_MASQ_REG_PROTO_MAX],
+			err = nft_parse_register_load(ctx, tb[NFTA_MASQ_REG_PROTO_MAX],
 						      &priv->sreg_proto_max,
 						      plen);
 			if (err < 0)
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 9139ce38ea7b..0214ad1ced2f 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -657,7 +657,7 @@ int nft_meta_set_init(const struct nft_ctx *ctx,
 	}
 
 	priv->len = len;
-	err = nft_parse_register_load(tb[NFTA_META_SREG], &priv->sreg, len);
+	err = nft_parse_register_load(ctx, tb[NFTA_META_SREG], &priv->sreg, len);
 	if (err < 0)
 		return err;
 
diff --git a/net/netfilter/nft_nat.c b/net/netfilter/nft_nat.c
index 808f5802c270..983dd937fe02 100644
--- a/net/netfilter/nft_nat.c
+++ b/net/netfilter/nft_nat.c
@@ -214,13 +214,13 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	priv->family = family;
 
 	if (tb[NFTA_NAT_REG_ADDR_MIN]) {
-		err = nft_parse_register_load(tb[NFTA_NAT_REG_ADDR_MIN],
+		err = nft_parse_register_load(ctx, tb[NFTA_NAT_REG_ADDR_MIN],
 					      &priv->sreg_addr_min, alen);
 		if (err < 0)
 			return err;
 
 		if (tb[NFTA_NAT_REG_ADDR_MAX]) {
-			err = nft_parse_register_load(tb[NFTA_NAT_REG_ADDR_MAX],
+			err = nft_parse_register_load(ctx, tb[NFTA_NAT_REG_ADDR_MAX],
 						      &priv->sreg_addr_max,
 						      alen);
 			if (err < 0)
@@ -234,13 +234,13 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 
 	plen = sizeof_field(struct nf_nat_range, min_proto.all);
 	if (tb[NFTA_NAT_REG_PROTO_MIN]) {
-		err = nft_parse_register_load(tb[NFTA_NAT_REG_PROTO_MIN],
+		err = nft_parse_register_load(ctx, tb[NFTA_NAT_REG_PROTO_MIN],
 					      &priv->sreg_proto_min, plen);
 		if (err < 0)
 			return err;
 
 		if (tb[NFTA_NAT_REG_PROTO_MAX]) {
-			err = nft_parse_register_load(tb[NFTA_NAT_REG_PROTO_MAX],
+			err = nft_parse_register_load(ctx, tb[NFTA_NAT_REG_PROTO_MAX],
 						      &priv->sreg_proto_max,
 						      plen);
 			if (err < 0)
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index 509011b1ef59..09da7a3f9f96 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -143,7 +143,7 @@ static int nft_objref_map_init(const struct nft_ctx *ctx,
 	if (!(set->flags & NFT_SET_OBJECT))
 		return -EINVAL;
 
-	err = nft_parse_register_load(tb[NFTA_OBJREF_SET_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_OBJREF_SET_SREG], &priv->sreg,
 				      set->klen);
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index 50429cbd42da..330609a76fb2 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -981,7 +981,7 @@ static int nft_payload_set_init(const struct nft_ctx *ctx,
 	}
 	priv->csum_type = csum_type;
 
-	return nft_parse_register_load(tb[NFTA_PAYLOAD_SREG], &priv->sreg,
+	return nft_parse_register_load(ctx, tb[NFTA_PAYLOAD_SREG], &priv->sreg,
 				       priv->len);
 }
 
diff --git a/net/netfilter/nft_queue.c b/net/netfilter/nft_queue.c
index b2b8127c8d43..44e6817e6e29 100644
--- a/net/netfilter/nft_queue.c
+++ b/net/netfilter/nft_queue.c
@@ -136,7 +136,7 @@ static int nft_queue_sreg_init(const struct nft_ctx *ctx,
 	struct nft_queue *priv = nft_expr_priv(expr);
 	int err;
 
-	err = nft_parse_register_load(tb[NFTA_QUEUE_SREG_QNUM],
+	err = nft_parse_register_load(ctx, tb[NFTA_QUEUE_SREG_QNUM],
 				      &priv->sreg_qnum, sizeof(u32));
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_range.c b/net/netfilter/nft_range.c
index 51ae64cd268f..ea382f7bbd78 100644
--- a/net/netfilter/nft_range.c
+++ b/net/netfilter/nft_range.c
@@ -83,7 +83,7 @@ static int nft_range_init(const struct nft_ctx *ctx, const struct nft_expr *expr
 		goto err2;
 	}
 
-	err = nft_parse_register_load(tb[NFTA_RANGE_SREG], &priv->sreg,
+	err = nft_parse_register_load(ctx, tb[NFTA_RANGE_SREG], &priv->sreg,
 				      desc_from.len);
 	if (err < 0)
 		goto err2;
diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
index a58bd8d291ff..6568cc264078 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -51,13 +51,13 @@ static int nft_redir_init(const struct nft_ctx *ctx,
 
 	plen = sizeof_field(struct nf_nat_range, min_proto.all);
 	if (tb[NFTA_REDIR_REG_PROTO_MIN]) {
-		err = nft_parse_register_load(tb[NFTA_REDIR_REG_PROTO_MIN],
+		err = nft_parse_register_load(ctx, tb[NFTA_REDIR_REG_PROTO_MIN],
 					      &priv->sreg_proto_min, plen);
 		if (err < 0)
 			return err;
 
 		if (tb[NFTA_REDIR_REG_PROTO_MAX]) {
-			err = nft_parse_register_load(tb[NFTA_REDIR_REG_PROTO_MAX],
+			err = nft_parse_register_load(ctx, tb[NFTA_REDIR_REG_PROTO_MAX],
 						      &priv->sreg_proto_max,
 						      plen);
 			if (err < 0)
diff --git a/net/netfilter/nft_tproxy.c b/net/netfilter/nft_tproxy.c
index 71412adb73d4..1b691393d8b1 100644
--- a/net/netfilter/nft_tproxy.c
+++ b/net/netfilter/nft_tproxy.c
@@ -254,14 +254,14 @@ static int nft_tproxy_init(const struct nft_ctx *ctx,
 	}
 
 	if (tb[NFTA_TPROXY_REG_ADDR]) {
-		err = nft_parse_register_load(tb[NFTA_TPROXY_REG_ADDR],
+		err = nft_parse_register_load(ctx, tb[NFTA_TPROXY_REG_ADDR],
 					      &priv->sreg_addr, alen);
 		if (err < 0)
 			return err;
 	}
 
 	if (tb[NFTA_TPROXY_REG_PORT]) {
-		err = nft_parse_register_load(tb[NFTA_TPROXY_REG_PORT],
+		err = nft_parse_register_load(ctx, tb[NFTA_TPROXY_REG_PORT],
 					      &priv->sreg_port, sizeof(u16));
 		if (err < 0)
 			return err;
-- 
2.44.2

[RFC nf-next 2/4] netfilter: nf_tables: allow loads only when register is initialized

[Florian Westphal]

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.

This resuts in a small increase of nft_ctx structure by sizeof(long).

To cope with hypothetical rulesets like the example above we could emit
on-demand "reg[x] = 0" store when generating the datapath blob in
nf_tables_commit_chain_prepare().  Followup patch adds this.

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

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 9a71fc20598b..3f06ec7dc500 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;
+	DECLARE_BITMAP(reg_inited, NFT_REG32_NUM);
 };
 
 enum nft_data_desc_flags {
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7437b38269a5..c59a32ab9145 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -146,6 +146,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;
+
+	bitmap_zero(ctx->reg_inited, NFT_REG32_NUM);
 }
 
 static struct nft_trans *nft_trans_alloc_gfp(const struct nft_ctx *ctx,
@@ -11105,8 +11107,8 @@ 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;
-	int err;
+	int err, invalid_reg;
+	u32 reg, next_register;
 
 	err = nft_parse_register(attr, &reg);
 	if (err < 0)
@@ -11116,11 +11118,36 @@ int nft_parse_register_load(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
+	next_register = DIV_ROUND_UP(len, NFT_REG32_SIZE) + reg;
+
+	/* Can't happen: nft_validate_register_load() should have failed */
+	if (WARN_ON_ONCE(next_register > NFT_REG32_NUM))
+		return -EINVAL;
+
+	/* find first register that did not see an earlier store. */
+	invalid_reg = find_next_zero_bit(ctx->reg_inited, NFT_REG32_NUM, reg);
+
+	/* invalid register within the range that we're loading from? */
+	if (invalid_reg < next_register)
+		return -ENODATA;
+
 	*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 registers = DIV_ROUND_UP(len, NFT_REG32_SIZE);
+	struct nft_ctx *ctx = (struct nft_ctx *)__ctx;
+
+	if (WARN_ON_ONCE(len == 0 || reg < 0))
+		return;
+
+	bitmap_set(ctx->reg_inited, reg, registers);
+}
+
 static int nft_validate_register_store(const struct nft_ctx *ctx,
 				       enum nft_registers reg,
 				       const struct nft_data *data,
@@ -11142,7 +11169,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;
@@ -11154,8 +11181,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.44.2

Re: [RFC nf-next 2/4] netfilter: nf_tables: allow loads only when register is initialized

[Pablo Neira Ayuso]

On Thu, Jun 27, 2024 at 03:53:22PM +0200, Florian Westphal wrote:
> @@ -11105,8 +11107,8 @@ 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;
> -	int err;
> +	int err, invalid_reg;
> +	u32 reg, next_register;
>  
>  	err = nft_parse_register(attr, &reg);
>  	if (err < 0)
> @@ -11116,11 +11118,36 @@ int nft_parse_register_load(const struct nft_ctx *ctx,
>  	if (err < 0)
>  		return err;
>  
> +	next_register = DIV_ROUND_UP(len, NFT_REG32_SIZE) + reg;
> +
> +	/* Can't happen: nft_validate_register_load() should have failed */
> +	if (WARN_ON_ONCE(next_register > NFT_REG32_NUM))
> +		return -EINVAL;
> +
> +	/* find first register that did not see an earlier store. */
> +	invalid_reg = find_next_zero_bit(ctx->reg_inited, NFT_REG32_NUM, reg);

Is this assuming that register allocation from userspace is done secuencially?

> +	/* invalid register within the range that we're loading from? */
> +	if (invalid_reg < next_register)
> +		return -ENODATA;
> +
>  	*sreg = reg;
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(nft_parse_register_load);

Re: [RFC nf-next 2/4] netfilter: nf_tables: allow loads only when register is initialized

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +	next_register = DIV_ROUND_UP(len, NFT_REG32_SIZE) + reg;
> > +
> > +	/* Can't happen: nft_validate_register_load() should have failed */
> > +	if (WARN_ON_ONCE(next_register > NFT_REG32_NUM))
> > +		return -EINVAL;
> > +
> > +	/* find first register that did not see an earlier store. */
> > +	invalid_reg = find_next_zero_bit(ctx->reg_inited, NFT_REG32_NUM, reg);
> 
> Is this assuming that register allocation from userspace is done secuencially?

No, that would be a bug.

Each set bit represents a register, if the bit is 1, the register
saw a store.

The above is the load check: load is from register "reg", and we
check the first reg that did not see a store (is 0), starting from
reg.  The result (register with undefined content) needs to be larger
than next_register, which is the register coming after the current
access (can be NFT_REG32_NUM, in that case no furhter registers exist
and access is ok).

> > +	/* invalid register within the range that we're loading from? */
> > +	if (invalid_reg < next_register)
> > +		return -ENODATA;
> > +

This means that in range the relevant access range
[reg,next_reg) the is at least on register that did not see a store.

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

[Florian Westphal]

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

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

[Pablo Neira Ayuso]

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.

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

[Florian Westphal]

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

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

[Pablo Neira Ayuso]

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

[RFC nf-next 4/4] netfilter: nf_tables: don't initialize registers in nft_do_chain()

[Florian Westphal]

revert commit 4c905f6740a3 ("netfilter: nf_tables: initialize registers in
nft_do_chain()").

Previous patch makes sure that loads from uninitialized registers are
detected from the control plane. in this case rule blob auto-zeroes
registers.  Thus the explicit zeroing is not needed anymore.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index a48d5f0e2f3e..75598520b0fa 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -256,7 +256,7 @@ nft_do_chain(struct nft_pktinfo *pkt, void *priv)
 	const struct net *net = nft_net(pkt);
 	const struct nft_expr *expr, *last;
 	const struct nft_rule_dp *rule;
-	struct nft_regs regs = {};
+	struct nft_regs regs;
 	unsigned int stackptr = 0;
 	struct nft_jumpstack jumpstack[NFT_JUMP_STACK_SIZE];
 	bool genbit = READ_ONCE(net->nft.gencursor);
-- 
2.44.2