[PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Keep a per-rule bitmask that tracks registers that have seen a store,
then reject loads when the accessed registers haven't been flagged.

This changes uabi contract, because we previously allowed this.
Neither nftables nor iptables-nft create such rules.

In case there is breakage, we could insert an 'store 0 to x'
immediate expression into the ruleset automatically, but this
isn't done here.

Let me know if you think the "refuse" approach is too risky.

Florian Westphal (3):
  netfilter: nf_tables: pass context structure to
    nft_parse_register_load
  netfilter: nf_tables: validate register loads never access unitialised
    registers
  netfilter: nf_tables: don't initialize registers in nft_do_chain()

 include/net/netfilter/nf_tables.h      |  4 ++-
 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          | 40 +++++++++++++++++++++++---
 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, 76 insertions(+), 42 deletions(-)

-- 
2.39.2

[PATCH nf-next 1/3] 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 3ed21d2d5659..246c4a4620ae 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -253,7 +253,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 09542951656c..4fd6bbb88cd2 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -10036,7 +10036,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 84eae7cabc67..6e94619df639 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 b66647a5a171..522b84460e69 100644
--- a/net/netfilter/nft_byteorder.c
+++ b/net/netfilter/nft_byteorder.c
@@ -138,7 +138,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 6eb21a4f5698..4cdcbb74c74d 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 b9c84499438b..614628099ac3 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -601,7 +601,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 274579b1696e..9589a72c2af6 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -211,7 +211,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;
@@ -222,7 +222,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 a54a7f772cec..e30cd98acc7d 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -509,7 +509,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 7b9d4d1bd17c..aa2a54fa6800 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 ee8d487b69c0..9833fa429e5e 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -91,7 +91,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 cecf8ab90e58..f0156b31b5cf 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -111,7 +111,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 b115d77fbbc7..c1a833aa3aa1 100644
--- a/net/netfilter/nft_masq.c
+++ b/net/netfilter/nft_masq.c
@@ -54,13 +54,13 @@ static int nft_masq_init(const struct nft_ctx *ctx,
 	}
 
 	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 e384e0de7a54..ef3f8ebf6409 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 5c29915ab028..9d606ce7dda0 100644
--- a/net/netfilter/nft_nat.c
+++ b/net/netfilter/nft_nat.c
@@ -208,13 +208,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)
@@ -228,13 +228,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 cb37169608ba..90a84b61cc36 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -141,7 +141,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 3a3c7746e88f..3055576aa9ed 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -916,7 +916,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 0566d6aaf1e5..d75772986d05 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 a70196ffcb1e..40035784ce83 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -50,13 +50,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 ea83f661417e..c82ec0f99d43 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.39.2

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

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

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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

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

[Florian Westphal]

This reverts commit 4c905f6740a3 ("netfilter: nf_tables: initialize registers in nft_do_chain()").
Previous patch makes sure that loads from uninitialized registers are
rejected from the control plane, so this zeroing isn't 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 4d0ce12221f6..b917cf00dda7 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.39.2

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Phil Sutter]

Hi!

On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> Keep a per-rule bitmask that tracks registers that have seen a store,
> then reject loads when the accessed registers haven't been flagged.
> 
> This changes uabi contract, because we previously allowed this.
> Neither nftables nor iptables-nft create such rules.

Did you consider keeping this bitmask on a per base-chain level? One had
to perform this for each base chain of a table upon each rule change and
traverse the tree of chains jumped to from there. I guess the huge
overhead disqualifies this, though.

Cheers, Phil

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > Keep a per-rule bitmask that tracks registers that have seen a store,
> > then reject loads when the accessed registers haven't been flagged.
> > 
> > This changes uabi contract, because we previously allowed this.
> > Neither nftables nor iptables-nft create such rules.
> 
> Did you consider keeping this bitmask on a per base-chain level? One had
> to perform this for each base chain of a table upon each rule change and
> traverse the tree of chains jumped to from there. I guess the huge
> overhead disqualifies this, though.

Yes, but its very hard task, because in that case we also need to prove
that a write *WILL* happen, rather than *might happen*.

Consider:

rule1:
ip protocol tcp iifname "eth0" ...
reg1 := ip protocol
cmp reg1
reg2 := meta iifname

rule2:
iifname "eth1" ...
cmp reg2 "eth0"

rule 2 has to be rejected because reg2 might be unitialized for != tcp.

Even if we can handle this some way, we now also need to revalidate the
ruleset on deletes, because we'd have to detect when a register write
we depend on goes away.

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Phil Sutter]

On Fri, May 05, 2023 at 03:46:55PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > > Keep a per-rule bitmask that tracks registers that have seen a store,
> > > then reject loads when the accessed registers haven't been flagged.
> > > 
> > > This changes uabi contract, because we previously allowed this.
> > > Neither nftables nor iptables-nft create such rules.
> > 
> > Did you consider keeping this bitmask on a per base-chain level? One had
> > to perform this for each base chain of a table upon each rule change and
> > traverse the tree of chains jumped to from there. I guess the huge
> > overhead disqualifies this, though.
> 
> Yes, but its very hard task, because in that case we also need to prove
> that a write *WILL* happen, rather than *might happen*.
> 
> Consider:
> 
> rule1:
> ip protocol tcp iifname "eth0" ...
> reg1 := ip protocol
> cmp reg1
> reg2 := meta iifname
> 
> rule2:
> iifname "eth1" ...
> cmp reg2 "eth0"
> 
> rule 2 has to be rejected because reg2 might be unitialized for != tcp.
> 
> Even if we can handle this some way, we now also need to revalidate the
> ruleset on deletes, because we'd have to detect when a register write
> we depend on goes away.

Ah, right. I forgot about "partial" rule execution again. Same thing
which broke expression reduction for us.

Maybe one could introduce a "chain optimizer" creating an initial
meta-rule which just populates registers with packet/meta data rules
may need. Not something I would want to rely upon regarding kernel info
leaks, though.

Cheers, Phil

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Pablo Neira Ayuso]

Hi Florian,

On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> Keep a per-rule bitmask that tracks registers that have seen a store,
> then reject loads when the accessed registers haven't been flagged.
> 
> This changes uabi contract, because we previously allowed this.
> Neither nftables nor iptables-nft create such rules.
> 
> In case there is breakage, we could insert an 'store 0 to x'
> immediate expression into the ruleset automatically, but this
> isn't done here.
> 
> Let me know if you think the "refuse" approach is too risky.

Might the NFT_BREAK case defeat this approach? Sequence is:

1) expression that writes on register hits NFT_BREAK (nothing is written)
2) expression that read from register, it reads uninitialized data.

From ruleset load step, we cannot know if the write fails, because it
is subject to NFT_BREAK.

Thanks.

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > Keep a per-rule bitmask that tracks registers that have seen a store,
> > then reject loads when the accessed registers haven't been flagged.
> > 
> > This changes uabi contract, because we previously allowed this.
> > Neither nftables nor iptables-nft create such rules.
> > 
> > In case there is breakage, we could insert an 'store 0 to x'
> > immediate expression into the ruleset automatically, but this
> > isn't done here.
> > 
> > Let me know if you think the "refuse" approach is too risky.
> 
> Might the NFT_BREAK case defeat this approach? Sequence is:
> 
> 1) expression that writes on register hits NFT_BREAK (nothing is written)
> 2) expression that read from register, it reads uninitialized data.
>
> From ruleset load step, we cannot know if the write fails, because it
> is subject to NFT_BREAK.

Yes, but its irrelevant: If 1) issues NFT_BREAK, 2) won't execute.

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Pablo Neira Ayuso]

On Fri, May 05, 2023 at 04:51:13PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > > Keep a per-rule bitmask that tracks registers that have seen a store,
> > > then reject loads when the accessed registers haven't been flagged.
> > > 
> > > This changes uabi contract, because we previously allowed this.
> > > Neither nftables nor iptables-nft create such rules.
> > > 
> > > In case there is breakage, we could insert an 'store 0 to x'
> > > immediate expression into the ruleset automatically, but this
> > > isn't done here.
> > > 
> > > Let me know if you think the "refuse" approach is too risky.
> > 
> > Might the NFT_BREAK case defeat this approach? Sequence is:
> > 
> > 1) expression that writes on register hits NFT_BREAK (nothing is written)
> > 2) expression that read from register, it reads uninitialized data.
> >
> > From ruleset load step, we cannot know if the write fails, because it
> > is subject to NFT_BREAK.
> 
> Yes, but its irrelevant: If 1) issues NFT_BREAK, 2) won't execute.

And register tracking is done per rule, given context is per rule too,
good.

I wonder if it is worth to move the bitmask away from nft_ctx, given
this structure is stored in the struct nft_trans, hence increasing the
size of this object which is not required at a later state, maybe
there is a need for a new container structure that store data useful
for the initial preparation step of the commit protocol.

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, May 05, 2023 at 04:51:13PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > > > Keep a per-rule bitmask that tracks registers that have seen a store,
> > > > then reject loads when the accessed registers haven't been flagged.
> > > > 
> > > > This changes uabi contract, because we previously allowed this.
> > > > Neither nftables nor iptables-nft create such rules.
> > > > 
> > > > In case there is breakage, we could insert an 'store 0 to x'
> > > > immediate expression into the ruleset automatically, but this
> > > > isn't done here.
> > > > 
> > > > Let me know if you think the "refuse" approach is too risky.
> > > 
> > > Might the NFT_BREAK case defeat this approach? Sequence is:
> > > 
> > > 1) expression that writes on register hits NFT_BREAK (nothing is written)
> > > 2) expression that read from register, it reads uninitialized data.
> > >
> > > From ruleset load step, we cannot know if the write fails, because it
> > > is subject to NFT_BREAK.
> > 
> > Yes, but its irrelevant: If 1) issues NFT_BREAK, 2) won't execute.
> 
> And register tracking is done per rule, given context is per rule too,
> good.
> 
> I wonder if it is worth to move the bitmask away from nft_ctx, given
> this structure is stored in the struct nft_trans, hence increasing the
> size of this object which is not required at a later state, maybe
> there is a need for a new container structure that store data useful
> for the initial preparation step of the commit protocol.

Hmm, this will get messy.

I only see two alternatives:

- place the bitmask in the pernet structure.
- add struct nft_expr_ctx as a container structure, which has
  nft_ctx as first member and the bitmask as second member, to
  be used for NEWRULE and NEWSETELEM instead of nft_ctx.

Any better idea?

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Pablo Neira Ayuso]

On Sun, May 07, 2023 at 01:22:54PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, May 05, 2023 at 04:51:13PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Fri, May 05, 2023 at 01:16:53PM +0200, Florian Westphal wrote:
> > > > > Keep a per-rule bitmask that tracks registers that have seen a store,
> > > > > then reject loads when the accessed registers haven't been flagged.
> > > > > 
> > > > > This changes uabi contract, because we previously allowed this.
> > > > > Neither nftables nor iptables-nft create such rules.
> > > > > 
> > > > > In case there is breakage, we could insert an 'store 0 to x'
> > > > > immediate expression into the ruleset automatically, but this
> > > > > isn't done here.
> > > > > 
> > > > > Let me know if you think the "refuse" approach is too risky.
> > > > 
> > > > Might the NFT_BREAK case defeat this approach? Sequence is:
> > > > 
> > > > 1) expression that writes on register hits NFT_BREAK (nothing is written)
> > > > 2) expression that read from register, it reads uninitialized data.
> > > >
> > > > From ruleset load step, we cannot know if the write fails, because it
> > > > is subject to NFT_BREAK.
> > > 
> > > Yes, but its irrelevant: If 1) issues NFT_BREAK, 2) won't execute.
> > 
> > And register tracking is done per rule, given context is per rule too,
> > good.
> > 
> > I wonder if it is worth to move the bitmask away from nft_ctx, given
> > this structure is stored in the struct nft_trans, hence increasing the
> > size of this object which is not required at a later state, maybe
> > there is a need for a new container structure that store data useful
> > for the initial preparation step of the commit protocol.
> 
> Hmm, this will get messy.
> 
> I only see two alternatives:
> 
> - place the bitmask in the pernet structure.
> - add struct nft_expr_ctx as a container structure, which has
>   nft_ctx as first member and the bitmask as second member, to
>   be used for NEWRULE and NEWSETELEM instead of nft_ctx.

Can the 'level' field be moved to this nft_expr_ctx structure? This
field is only used from the preparation phase (not in the commit
phase).

Probably we need to rename nft_ctx to nft_trans_ctx, so it contains
the fields that are needed from the commit phase. Then, re-add a
nft_ctx again which contains nft_trans_ctx at the beginning, then the
register bitmap and the level field. Thus, any future fields only
required by preparation phase only will go in nft_ctx, and fields that
are specifically are set up from preparation phase and consumed from
commit step go in nft_trans_ctx.

It is a bit of churn, but it is probably good to tidy up this for
future extensions?

Let me know, thanks.

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hmm, this will get messy.
> > 
> > I only see two alternatives:
> > 
> > - place the bitmask in the pernet structure.
> > - add struct nft_expr_ctx as a container structure, which has
> >   nft_ctx as first member and the bitmask as second member, to
> >   be used for NEWRULE and NEWSETELEM instead of nft_ctx.
> 
> Can the 'level' field be moved to this nft_expr_ctx structure? This
> field is only used from the preparation phase (not in the commit
> phase).
> 
> Probably we need to rename nft_ctx to nft_trans_ctx, so it contains
> the fields that are needed from the commit phase. Then, re-add a
> nft_ctx again which contains nft_trans_ctx at the beginning, then the
> register bitmap and the level field. Thus, any future fields only
> required by preparation phase only will go in nft_ctx, and fields that
> are specifically are set up from preparation phase and consumed from
> commit step go in nft_trans_ctx.
> 
> It is a bit of churn, but it is probably good to tidy up this for
> future extensions?

Yes, its a lot of churn, I can have a look at how intrusive this will
be.  Problem is that we have a bunch of helpers that take
'struct nft_ctx *', which are fed via '&trans->ctx'.

I'd like to avoid 'union nf_ctx_any *' tricks...

Re: [PATCH nf-next 0/3] netfilter: nf_tables: reject loads from uninitialized registers

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Hmm, this will get messy.
> > > 
> > > I only see two alternatives:
> > > 
> > > - place the bitmask in the pernet structure.
> > > - add struct nft_expr_ctx as a container structure, which has
> > >   nft_ctx as first member and the bitmask as second member, to
> > >   be used for NEWRULE and NEWSETELEM instead of nft_ctx.
> > 
> > Can the 'level' field be moved to this nft_expr_ctx structure? This
> > field is only used from the preparation phase (not in the commit
> > phase).
> > 
> > Probably we need to rename nft_ctx to nft_trans_ctx, so it contains
> > the fields that are needed from the commit phase. Then, re-add a
> > nft_ctx again which contains nft_trans_ctx at the beginning, then the
> > register bitmap and the level field. Thus, any future fields only
> > required by preparation phase only will go in nft_ctx, and fields that
> > are specifically are set up from preparation phase and consumed from
> > commit step go in nft_trans_ctx.
> > 
> > It is a bit of churn, but it is probably good to tidy up this for
> > future extensions?
> 
> Yes, its a lot of churn, I can have a look at how intrusive this will
> be.  Problem is that we have a bunch of helpers that take
> 'struct nft_ctx *', which are fed via '&trans->ctx'.
> 
> I'd like to avoid 'union nf_ctx_any *' tricks...

I would prefer not to do this.

I would have to change ->init for expressions and objects, and ->validate
too.

I would have to change ->walk() api in sets to get a
ctx-that-has-level/reg_inited fields.

But ->walk() is used in dumps too.

Compared to before this series, size increase of ctx is 48 -> 56 bytes.
This is rounded to 64 byte slab anyway, or 96 or 128 for transaction
structs that contain more data.  So this change doesn't buy anything
now.

In case we get further fields that are only relevant at
->validate/->init time and size indeed becomes a problem I'd propose to
instead add

struct nft_ctx_scratch {
	u8 level;
	DECLARE_BITMAP(reg_inited, NFT_REG32_COUNT);

	/* more temporay foo here */
;

and a 'struct nft_ctx_scratch *' member to existing nft_ctx struct.

But an even better argument to keep things as is:

If we need to adopt a lazy implicit-register-clearing in the future,
then we'd need the 'reg_inited' member in the transaction phase, so
that when the data rule blob is generated we can make sure the blob
generator can add the required 'reg := 0' store at the start of the
chain.

The only alternative that I can investigate if you like is to
add

'struct nft_trans_ctx' and then add some kind of
'nft_trans_ctx to nft_ctx' converter function, i.e. no longer
allow to do things like:

nf_tables_flowtable_notify(&trans->ctx, ..

but require sometung like

struct nft_ctx ctx;

nft_make_ctx_from_trans(&ctx, trans->saved_ctx);
nf_tables_flowtable_notify(&ctx, ..);

Downside is that we'd have silent information loss because
the 'additional late fields' would always be 0.

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

[Pablo Neira Ayuso]

Hi Florian,

On Fri, May 05, 2023 at 01:16:55PM +0200, Florian Westphal wrote:
> 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.

We can probably achieve the same effect by recovering the runtime
register tracking patches I posted. It should be possible to add
unlikely() to the branch that checks for uninitialized data in source
register, that is missing in this patch:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-6-pablo@netfilter.org/

such patch also equires these patches to add the helpers to load and
store:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-3-pablo@netfilter.org/
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-4-pablo@netfilter.org/
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-5-pablo@netfilter.org/

I think those helpers to load and store on registers should have been
there since the beginning instead of opencoding operations on the
registers. I am going to collect some numbers with these patches
including unlikely() to the _load() checks. I fear I might have
introduced some subtle bug, I remember these patches are passing
selftests fine, but I am not sure we have enough of these selftests.

As you suggested, I also considered using the new track infrastructure
(the one I posted to achieve the combo expressions) to detect a read
to uninitialized registers from control plane, but it gets complicated
again because:

1) what level should register tracking happen at? rule, chain or from
   basechain to leaf chains (to ensure that we retain the freedom to
   make more transformation from userspace, eg. static flag for ruleset
   that never change to omit redundant operations in the generated
   bytecode). Your patch selects rule level. Chain level will lose
   context when jumping/going to another chain. Inspecting from
   basechain to leaf chains will be expensive in dynamic rulesets.

2) combo expressions omit the register store operation, the tracking
   infrastructure would need to distinguish between two situations:
   register data has been omitted or register data is missing because
   userspace provides bytecode that tries to read uninitialized
   registers.

While I agree control plane is ideal for this, because it allows to
reject a ruleset that reads on uninitialized register data, checking
at rule/chain level cripples expressiveness in a way that it will not
be easy to revert in the future if we want to change direction.

> Neither nftables nor iptables-nft generate such rules, each rule is
> always standalone.

That is true these days indeed. I like your approach because it is
simple. But my concern is that this limits expressiveness.

Thanks.

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >     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.
> 
> We can probably achieve the same effect by recovering the runtime
> register tracking patches I posted. It should be possible to add
> unlikely() to the branch that checks for uninitialized data in source
> register, that is missing in this patch:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-6-pablo@netfilter.org/
> 
> such patch also equires these patches to add the helpers to load and
> store:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-3-pablo@netfilter.org/
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-4-pablo@netfilter.org/
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230505153130.2385-5-pablo@netfilter.org/
> 
> I think those helpers to load and store on registers should have been
> there since the beginning instead of opencoding operations on the
> registers. I am going to collect some numbers with these patches
> including unlikely() to the _load() checks. I fear I might have
> introduced some subtle bug, I remember these patches are passing
> selftests fine, but I am not sure we have enough of these selftests.

Thanks, I agree that we will need runtime checking because control plane
is too complex due to stores becoming unreliable (NFT_BREAK
encountered) if we want to do this load suppression.

I get the impression that we're overthinking this, we really
should not bother too much with speeding up iptables-style linear
rulesets.

I'm only concerned with speeding up compact rulesets that use
sets/maps wherever possible.

I think for those getting rid of the memset() will help more
than eliding a couple of reloads.

> 1) what level should register tracking happen at? rule, chain or from
>    basechain to leaf chains (to ensure that we retain the freedom to
>    make more transformation from userspace, eg. static flag for ruleset
>    that never change to omit redundant operations in the generated
>    bytecode). Your patch selects rule level. Chain level will lose
>    context when jumping/going to another chain. Inspecting from
>    basechain to leaf chains will be expensive in dynamic rulesets.

Right.  I used rule level because its easy to do but as you say,
it prevents userspace from ever making a "clever" ruleset that
performs any sort of preload (without kernel update).

> 2) combo expressions omit the register store operation, the tracking
>    infrastructure would need to distinguish between two situations:
>    register data has been omitted or register data is missing because
>    userspace provides bytecode that tries to read uninitialized
>    registers.
> 
> While I agree control plane is ideal for this, because it allows to
> reject a ruleset that reads on uninitialized register data, checking
> at rule/chain level cripples expressiveness in a way that it will not
> be easy to revert in the future if we want to change direction.
> 
> > Neither nftables nor iptables-nft generate such rules, each rule is
> > always standalone.
> 
> That is true these days indeed. I like your approach because it is
> simple. But my concern is that this limits expressiveness.

Lets wait for your test with runtime checking, so we have some data
on how much of a slowdown that gives.

But I'd like to see some proof that a *good* ruleset has many
redundant loads where such cross-rule load elimination can add a
benefit in the first place.  Else doing runtime validation makes no
sense.

What I can think of is to allow this patch in,
i.e. rule level enforcement, and then follow a approach similar to what
you already proposed earlier: A per-chain prefetch stage.

This would mean instead of

chain [ rule [ expr, expr, expr ]] [ rule [ expr , expr ]] ..

we'd have
chain prefetch [ expr, expr ] [ rule ... ]

The prefetch is limited to selected meta and payload operations.
Failure means entire chain is bypassed.

Any sort of jump invalidates the prefetch.

So, this patch would be later relaxed to pre-init
the "prefetch" registers as "initialised", so following is legal:

prefetch: reg3: meta l4roto, reg4: meta load iif
rule 1: reg2 := ip saddr, lookup(reg2 . reg3 .reg 4) accept
rule 2: reg3 == icmp accept ...  // no longer rejected as uninitialized
rule 3: reg4 == "eth*" jump ...
rule 4: reg3 == icmp accept ...  // fails --> prefetch lost
rule 4: reg2 := ip saddr ...  // fails --> prefetch overwritten

Yet another idea: Introduce internal shadow registers:
Prefetch to reg1, reg2, reg3 will auto-store those *also*
to *pfr1*, *pfr2, and so on.

This would allow register content recovery after each jump:
reg1 = pfr1
reg2 = pfr2

and so on.

But again, I think this is the wrong approach and we should not
bother with load elimination or anything else that will complicate
the data path.

Combo match approach is good because it doesn't have that issue.