[PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Pablo Neira Ayuso]

Several matches and one target check that the hook is correct from
checkentry(), however, the basechain is only available from
nft_table_validate().

This patch calls checkentry() for matches and targets from the
nft_compat expression .validate path for the following matches/target:

- addrtype
- devgroup
- physdev
- policy
- set
- TCPMSS

The xt_match .destroy indirection is also called to restore the
xt_set refcounts that is performed by .checkentry. The remaining
extensions provide no .destroy interface.

This patch sets the table in the nft_ctx struct in nft_table_validate()
which is required by this patch.

The nft_compat_check_match() and nft_compat_check_target() helper
functions are added to wrap common code used from .init and .validate
path.

The protocol and inverse flags are set to always match from the
expression .validate path, this is already checked from the init path.

Fixes: 0ca743a55991 ("netfilter: nf_tables: add compatibility layer for x_tables")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
A more self-contained alternative to Florian's:
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260419104509.42196-1-fw@strlen.de/

 net/netfilter/nf_tables_api.c |   1 +
 net/netfilter/nft_compat.c    | 118 ++++++++++++++++++++++++++++------
 2 files changed, 100 insertions(+), 19 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 00ccc0734ac9..070e97efe7d1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -4196,6 +4196,7 @@ static int nft_table_validate(struct net *net, const struct nft_table *table)
 	struct nft_chain *chain;
 	struct nft_ctx ctx = {
 		.net	= net,
+		.table	= (struct nft_table *)table,
 		.family	= table->family,
 	};
 	int err = 0;
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index decc725a33c2..2352505fb992 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -229,6 +229,20 @@ static int nft_parse_compat(const struct nlattr *attr, u16 *proto, bool *inv)
 	return 0;
 }
 
+static int nft_compat_check_target(const struct nft_ctx *ctx,
+				   const struct nft_expr *expr,
+				   void *info, size_t size,
+				   u16 proto, bool inv)
+{
+	struct xt_target *target = expr->ops->data;
+	struct xt_tgchk_param par;
+	union nft_entry e = {};
+
+	nft_target_set_tgchk_param(&par, ctx, target, info, &e, proto, inv);
+
+	return xt_check_target(&par, size, proto, inv);
+}
+
 static void nft_compat_wait_for_destructors(struct net *net)
 {
 	/* xtables matches or targets can have side effects, e.g.
@@ -244,13 +258,11 @@ static int
 nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 		const struct nlattr * const tb[])
 {
-	void *info = nft_expr_priv(expr);
 	struct xt_target *target = expr->ops->data;
-	struct xt_tgchk_param par;
 	size_t size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO]));
-	u16 proto = 0;
+	void *info = nft_expr_priv(expr);
 	bool inv = false;
-	union nft_entry e = {};
+	u16 proto = 0;
 	int ret;
 
 	target_compat_from_user(target, nla_data(tb[NFTA_TARGET_INFO]), info);
@@ -261,11 +273,9 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 			return ret;
 	}
 
-	nft_target_set_tgchk_param(&par, ctx, target, info, &e, proto, inv);
-
 	nft_compat_wait_for_destructors(ctx->net);
 
-	ret = xt_check_target(&par, size, proto, inv);
+	ret = nft_compat_check_target(ctx, expr, info, size, proto, inv);
 	if (ret < 0) {
 		if (ret == -ENOENT) {
 			const char *modname = NULL;
@@ -382,6 +392,26 @@ static int nft_target_validate(const struct nft_ctx *ctx,
 		if (target->hooks && !(hook_mask & target->hooks))
 			return -EINVAL;
 
+		/* At least one target needs to validate hooks at checkentry()
+		 * stage because such validation depends on the match
+		 * configuration. This cannot be enabled for all matches,
+		 * because some of them perform more than simple validation,
+		 * such as bumping reference counter on objects.
+		 */
+		if (!strcmp(target->name, "TCPMSS")) {
+			struct xt_target *target = expr->ops->data;
+			size_t size = XT_ALIGN(target->targetsize);
+			void *info = nft_expr_priv(expr);
+
+			/* nft_target_init() already checked for protocol and
+			 * inverse, not available in this patch, lie here.
+			 */
+			ret = nft_compat_check_target(ctx, expr, info, size,
+						      target->proto, false);
+			if (ret < 0)
+				return ret;
+		}
+
 		ret = nft_compat_chain_validate_dependency(ctx, target->table);
 		if (ret < 0)
 			return ret;
@@ -494,17 +524,29 @@ static void match_compat_from_user(struct xt_match *m, void *in, void *out)
 		memset(out + m->matchsize, 0, pad);
 }
 
+static int nft_compat_check_match(const struct nft_ctx *ctx,
+				  const struct nft_expr *expr,
+				  void *info, size_t size,
+				  u16 proto, bool inv)
+{
+	struct xt_match *match = expr->ops->data;
+	struct xt_mtchk_param par;
+	union nft_entry e = {};
+
+	nft_match_set_mtchk_param(&par, ctx, match, info, &e, proto, inv);
+
+	return xt_check_match(&par, size, proto, inv);
+}
+
 static int
 __nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 		 const struct nlattr * const tb[],
 		 void *info)
 {
-	struct xt_match *match = expr->ops->data;
-	struct xt_mtchk_param par;
 	size_t size = XT_ALIGN(nla_len(tb[NFTA_MATCH_INFO]));
-	u16 proto = 0;
+	struct xt_match *match = expr->ops->data;
 	bool inv = false;
-	union nft_entry e = {};
+	u16 proto = 0;
 	int ret;
 
 	match_compat_from_user(match, nla_data(tb[NFTA_MATCH_INFO]), info);
@@ -515,11 +557,9 @@ __nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 			return ret;
 	}
 
-	nft_match_set_mtchk_param(&par, ctx, match, info, &e, proto, inv);
-
 	nft_compat_wait_for_destructors(ctx->net);
 
-	return xt_check_match(&par, size, proto, inv);
+	return nft_compat_check_match(ctx, expr, info, size, proto, inv);
 }
 
 static int
@@ -547,12 +587,9 @@ nft_match_large_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	return ret;
 }
 
-static void
-__nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr,
-		    void *info)
+static void nft_mt_destroy(const struct nft_ctx *ctx, struct xt_match *match,
+			   void *info)
 {
-	struct xt_match *match = expr->ops->data;
-	struct module *me = match->me;
 	struct xt_mtdtor_param par;
 
 	par.net = ctx->net;
@@ -561,7 +598,16 @@ __nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	par.family = ctx->family;
 	if (par.match->destroy != NULL)
 		par.match->destroy(&par);
+}
 
+static void
+__nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr,
+		    void *info)
+{
+	struct xt_match *match = expr->ops->data;
+	struct module *me = match->me;
+
+	nft_mt_destroy(ctx, match, info);
 	__nft_mt_tg_destroy(me, expr);
 }
 
@@ -643,6 +689,40 @@ static int nft_match_validate(const struct nft_ctx *ctx,
 		if (match->hooks && !(hook_mask & match->hooks))
 			return -EINVAL;
 
+		/* Several matches need to validate hooks at checkentry() stage
+		 * because such validation depends on the match configuration.
+		 */
+		if (!strcmp(match->name, "addrtype") ||
+		    !strcmp(match->name, "devgroup") ||
+		    !strcmp(match->name, "physdev") ||
+		    !strcmp(match->name, "policy") ||
+		    !strcmp(match->name, "set")) {
+			struct xt_match *match = expr->ops->data;
+			size_t size = XT_ALIGN(match->matchsize);
+			void *info;
+
+			if (NFT_EXPR_SIZE(size) > NFT_MATCH_LARGE_THRESH) {
+				struct nft_xt_match_priv *priv = nft_expr_priv(expr);
+
+				info = priv->info;
+			} else {
+				info = nft_expr_priv(expr);
+			}
+
+			/* __nft_match_init() already checked for protocol and
+			 * inverse, not available in this patch, lie here.
+			 */
+			ret = nft_compat_check_match(ctx, expr, info, size,
+						     match->proto, false);
+			if (ret < 0)
+				return ret;
+
+			 /* The set match bumps reference count, restore after
+			  * this checkentry call.
+			  */
+			nft_mt_destroy(ctx, match, info);
+		}
+
 		ret = nft_compat_chain_validate_dependency(ctx, match->table);
 		if (ret < 0)
 			return ret;
-- 
2.47.3

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Several matches and one target check that the hook is correct from
> checkentry(), however, the basechain is only available from
> nft_table_validate().
> 
> This patch calls checkentry() for matches and targets from the
> nft_compat expression .validate path for the following matches/target:

I worry that this is fragile.  Not all ->checkentry callbacks are pure.
Some create /proc entries or bump reference counts.


> +		if (!strcmp(target->name, "TCPMSS")) {

This is missing "SET".

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Pablo Neira Ayuso]

On Mon, Apr 20, 2026 at 07:55:22PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Several matches and one target check that the hook is correct from
> > checkentry(), however, the basechain is only available from
> > nft_table_validate().
> > 
> > This patch calls checkentry() for matches and targets from the
> > nft_compat expression .validate path for the following matches/target:
> 
> I worry that this is fragile.  Not all ->checkentry callbacks are pure.
> Some create /proc entries or bump reference counts.

xt_set does bump the reference count. This calls xt.destroy to restore it.
I am only calling them for the list of expression you mentioned.

> > +		if (!strcmp(target->name, "TCPMSS")) {
> 
> This is missing "SET".

I can add this.

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Apr 20, 2026 at 07:55:22PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Several matches and one target check that the hook is correct from
> > > checkentry(), however, the basechain is only available from
> > > nft_table_validate().
> > > 
> > > This patch calls checkentry() for matches and targets from the
> > > nft_compat expression .validate path for the following matches/target:
> > 
> > I worry that this is fragile.  Not all ->checkentry callbacks are pure.
> > Some create /proc entries or bump reference counts.
> 
> xt_set does bump the reference count. This calls xt.destroy to restore it.
> I am only calling them for the list of expression you mentioned.

I worry this will lead to trouble later, e.g. info->priv = kmalloc( ...)
-> memory leak.

But OK, at least there is a test case in iptables.git for this.

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Pablo Neira Ayuso]

On Mon, Apr 20, 2026 at 08:21:18PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Apr 20, 2026 at 07:55:22PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > Several matches and one target check that the hook is correct from
> > > > checkentry(), however, the basechain is only available from
> > > > nft_table_validate().
> > > > 
> > > > This patch calls checkentry() for matches and targets from the
> > > > nft_compat expression .validate path for the following matches/target:
> > > 
> > > I worry that this is fragile.  Not all ->checkentry callbacks are pure.
> > > Some create /proc entries or bump reference counts.
> > 
> > xt_set does bump the reference count. This calls xt.destroy to restore it.
> > I am only calling them for the list of expression you mentioned.
> 
> I worry this will lead to trouble later, e.g. info->priv = kmalloc( ...)
> -> memory leak.

If someone needs to cover for more extensions, they will have to
update the list of extensions covered by the strcmp() check on the
extension name.

> But OK, at least there is a test case in iptables.git for this.

Yes.

Your approach duplicates .checkentry in some way, you have to make
sure what your .validate and .checkentry perform the same check, ie.
they are in sync.

The approach proposed in this patch is not universal, because
checkentry() is a place where many things happen as you suggested
(/proc entries being registered, reference count being bumped,i
kmalloc...).

If this needs to be generalized further, maybe checkentry() needs to
extended to improve integration with nftables.

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Your approach duplicates .checkentry in some way, you have to make
> sure what your .validate and .checkentry perform the same check, ie.
> they are in sync.

Thats why I updated the affected .checkentry functions to use
the validate functions internally -- to make sure the code is called
even for classic iptables.

> If this needs to be generalized further, maybe checkentry() needs to
> extended to improve integration with nftables.

I hope not.  But I don't care, if you prefer your patch then so be it.

I just find it sad we duplicate efforts all the time.

Re: [PATCH nf] netfilter: nft_compat: run checkentry() from .validate

[Pablo Neira Ayuso]

On Mon, Apr 20, 2026 at 08:53:19PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Your approach duplicates .checkentry in some way, you have to make
> > sure what your .validate and .checkentry perform the same check, ie.
> > they are in sync.
> 
> Thats why I updated the affected .checkentry functions to use
> the validate functions internally -- to make sure the code is called
> even for classic iptables.
> 
> > If this needs to be generalized further, maybe checkentry() needs to
> > extended to improve integration with nftables.
> 
> I hope not.  But I don't care, if you prefer your patch then so be it.

I can toss this patch if you prefer, I will post it complete then
decide what to do.

> I just find it sad we duplicate efforts all the time.

I thought I could provide a more self-contained approach for this
nft_compat specific bug.