[nf-next PATCH v2] netfilter: nf_tables: Fix for extra data in delete notifications

[nf-next PATCH v2] netfilter: nf_tables: Fix for extra data in delete notifications

[Phil Sutter]

All routines modified in this patch conditionally return early depending
on event value (and other criteria, i.e., chain/flowtable updates).
These checks were defeated by an upfront modification of that variable
for use in nfnl_msg_put(). Restore functionality by avoiding the
modification.

This change is particularly important for user space to distinguish
between a chain/flowtable update removing a hook and full deletion.

Fixes: 28339b21a365 ("netfilter: nf_tables: do not send complete notification of deletions")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Channeling this through -next despite it being a fix since unpatched
nft monitor chokes on the shortened delete flowtable notifications.

Changes since v1:
- User space depends on NFTA_OBJ_TYPE for proper deserialization, do not
  skip that attribute.
---
 net/netfilter/nf_tables_api.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 8952b50b0224..9bf003797355 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1153,9 +1153,9 @@ static int nf_tables_fill_table_info(struct sk_buff *skb, struct net *net,
 {
 	struct nlmsghdr *nlh;
 
-	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
-	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
-			   NFNETLINK_V0, nft_base_seq(net));
+	nlh = nfnl_msg_put(skb, portid, seq,
+			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
+			   flags, family, NFNETLINK_V0, nft_base_seq(net));
 	if (!nlh)
 		goto nla_put_failure;
 
@@ -2020,9 +2020,9 @@ static int nf_tables_fill_chain_info(struct sk_buff *skb, struct net *net,
 {
 	struct nlmsghdr *nlh;
 
-	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
-	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
-			   NFNETLINK_V0, nft_base_seq(net));
+	nlh = nfnl_msg_put(skb, portid, seq,
+			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
+			   flags, family, NFNETLINK_V0, nft_base_seq(net));
 	if (!nlh)
 		goto nla_put_failure;
 
@@ -4851,9 +4851,10 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
 	u32 seq = ctx->seq;
 	int i;
 
-	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
-	nlh = nfnl_msg_put(skb, portid, seq, event, flags, ctx->family,
-			   NFNETLINK_V0, nft_base_seq(ctx->net));
+	nlh = nfnl_msg_put(skb, portid, seq,
+			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
+			   flags, ctx->family, NFNETLINK_V0,
+			   nft_base_seq(ctx->net));
 	if (!nlh)
 		goto nla_put_failure;
 
@@ -8346,14 +8347,15 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
 {
 	struct nlmsghdr *nlh;
 
-	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
-	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
-			   NFNETLINK_V0, nft_base_seq(net));
+	nlh = nfnl_msg_put(skb, portid, seq,
+			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
+			   flags, family, NFNETLINK_V0, nft_base_seq(net));
 	if (!nlh)
 		goto nla_put_failure;
 
 	if (nla_put_string(skb, NFTA_OBJ_TABLE, table->name) ||
 	    nla_put_string(skb, NFTA_OBJ_NAME, obj->key.name) ||
+	    nla_put_be32(skb, NFTA_OBJ_TYPE, htonl(obj->ops->type->type)) ||
 	    nla_put_be64(skb, NFTA_OBJ_HANDLE, cpu_to_be64(obj->handle),
 			 NFTA_OBJ_PAD))
 		goto nla_put_failure;
@@ -8363,8 +8365,7 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
 		return 0;
 	}
 
-	if (nla_put_be32(skb, NFTA_OBJ_TYPE, htonl(obj->ops->type->type)) ||
-	    nla_put_be32(skb, NFTA_OBJ_USE, htonl(obj->use)) ||
+	if (nla_put_be32(skb, NFTA_OBJ_USE, htonl(obj->use)) ||
 	    nft_object_dump(skb, NFTA_OBJ_DATA, obj, reset))
 		goto nla_put_failure;
 
@@ -9391,9 +9392,9 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 	struct nft_hook *hook;
 	struct nlmsghdr *nlh;
 
-	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
-	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
-			   NFNETLINK_V0, nft_base_seq(net));
+	nlh = nfnl_msg_put(skb, portid, seq,
+			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
+			   flags, family, NFNETLINK_V0, nft_base_seq(net));
 	if (!nlh)
 		goto nla_put_failure;
 
-- 
2.49.0

Re: [nf-next PATCH v2] netfilter: nf_tables: Fix for extra data in delete notifications

[Pablo Neira Ayuso]

On Thu, Jun 12, 2025 at 08:30:24PM +0200, Phil Sutter wrote:
> All routines modified in this patch conditionally return early depending
> on event value (and other criteria, i.e., chain/flowtable updates).
> These checks were defeated by an upfront modification of that variable
> for use in nfnl_msg_put(). Restore functionality by avoiding the
> modification.

Thanks for fixing this.

> This change is particularly important for user space to distinguish
> between a chain/flowtable update removing a hook and full deletion.
> 
> Fixes: 28339b21a365 ("netfilter: nf_tables: do not send complete notification of deletions")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Channeling this through -next despite it being a fix since unpatched
> nft monitor chokes on the shortened delete flowtable notifications.

I am afraid this patch will end up in -stable, breaking userspace, how
bad is the choking? Maybe 28339b21a365 needs to be reverted, then fix
userspace to prepare for it and re-add it in nf-next?

Not sure what path to follow with this.

> Changes since v1:
> - User space depends on NFTA_OBJ_TYPE for proper deserialization, do not
>   skip that attribute.
> ---
>  net/netfilter/nf_tables_api.c | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 8952b50b0224..9bf003797355 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1153,9 +1153,9 @@ static int nf_tables_fill_table_info(struct sk_buff *skb, struct net *net,
>  {
>  	struct nlmsghdr *nlh;
>  
> -	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
> -	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
> -			   NFNETLINK_V0, nft_base_seq(net));
> +	nlh = nfnl_msg_put(skb, portid, seq,
> +			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
> +			   flags, family, NFNETLINK_V0, nft_base_seq(net));
>  	if (!nlh)
>  		goto nla_put_failure;
>  
> @@ -2020,9 +2020,9 @@ static int nf_tables_fill_chain_info(struct sk_buff *skb, struct net *net,
>  {
>  	struct nlmsghdr *nlh;
>  
> -	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
> -	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
> -			   NFNETLINK_V0, nft_base_seq(net));
> +	nlh = nfnl_msg_put(skb, portid, seq,
> +			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
> +			   flags, family, NFNETLINK_V0, nft_base_seq(net));
>  	if (!nlh)
>  		goto nla_put_failure;
>  
> @@ -4851,9 +4851,10 @@ static int nf_tables_fill_set(struct sk_buff *skb, const struct nft_ctx *ctx,
>  	u32 seq = ctx->seq;
>  	int i;
>  
> -	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
> -	nlh = nfnl_msg_put(skb, portid, seq, event, flags, ctx->family,
> -			   NFNETLINK_V0, nft_base_seq(ctx->net));
> +	nlh = nfnl_msg_put(skb, portid, seq,
> +			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
> +			   flags, ctx->family, NFNETLINK_V0,
> +			   nft_base_seq(ctx->net));
>  	if (!nlh)
>  		goto nla_put_failure;
>  
> @@ -8346,14 +8347,15 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
>  {
>  	struct nlmsghdr *nlh;
>  
> -	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
> -	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
> -			   NFNETLINK_V0, nft_base_seq(net));
> +	nlh = nfnl_msg_put(skb, portid, seq,
> +			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
> +			   flags, family, NFNETLINK_V0, nft_base_seq(net));
>  	if (!nlh)
>  		goto nla_put_failure;
>  
>  	if (nla_put_string(skb, NFTA_OBJ_TABLE, table->name) ||
>  	    nla_put_string(skb, NFTA_OBJ_NAME, obj->key.name) ||
> +	    nla_put_be32(skb, NFTA_OBJ_TYPE, htonl(obj->ops->type->type)) ||
>  	    nla_put_be64(skb, NFTA_OBJ_HANDLE, cpu_to_be64(obj->handle),
>  			 NFTA_OBJ_PAD))
>  		goto nla_put_failure;
> @@ -8363,8 +8365,7 @@ static int nf_tables_fill_obj_info(struct sk_buff *skb, struct net *net,
>  		return 0;
>  	}
>  
> -	if (nla_put_be32(skb, NFTA_OBJ_TYPE, htonl(obj->ops->type->type)) ||
> -	    nla_put_be32(skb, NFTA_OBJ_USE, htonl(obj->use)) ||
> +	if (nla_put_be32(skb, NFTA_OBJ_USE, htonl(obj->use)) ||
>  	    nft_object_dump(skb, NFTA_OBJ_DATA, obj, reset))
>  		goto nla_put_failure;
>  
> @@ -9391,9 +9392,9 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
>  	struct nft_hook *hook;
>  	struct nlmsghdr *nlh;
>  
> -	event = nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event);
> -	nlh = nfnl_msg_put(skb, portid, seq, event, flags, family,
> -			   NFNETLINK_V0, nft_base_seq(net));
> +	nlh = nfnl_msg_put(skb, portid, seq,
> +			   nfnl_msg_type(NFNL_SUBSYS_NFTABLES, event),
> +			   flags, family, NFNETLINK_V0, nft_base_seq(net));
>  	if (!nlh)
>  		goto nla_put_failure;
>  
> -- 
> 2.49.0
> 

Re: [nf-next PATCH v2] netfilter: nf_tables: Fix for extra data in delete notifications

[Phil Sutter]

On Fri, Jun 13, 2025 at 12:42:18AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 12, 2025 at 08:30:24PM +0200, Phil Sutter wrote:
> > All routines modified in this patch conditionally return early depending
> > on event value (and other criteria, i.e., chain/flowtable updates).
> > These checks were defeated by an upfront modification of that variable
> > for use in nfnl_msg_put(). Restore functionality by avoiding the
> > modification.
> 
> Thanks for fixing this.

Took me more than a moment to notice! I guess 'var = func(var)' is
convenient, but also bad practice. :)

> > This change is particularly important for user space to distinguish
> > between a chain/flowtable update removing a hook and full deletion.
> > 
> > Fixes: 28339b21a365 ("netfilter: nf_tables: do not send complete notification of deletions")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Channeling this through -next despite it being a fix since unpatched
> > nft monitor chokes on the shortened delete flowtable notifications.
> 
> I am afraid this patch will end up in -stable, breaking userspace, how
> bad is the choking? Maybe 28339b21a365 needs to be reverted, then fix
> userspace to prepare for it and re-add it in nf-next?

Oh right, the Fixes: tag will probably cause that. User space segfaults
dereferencing a NULL-ptr. Happens in netlink_delinearize_{obj,flowtable}
which are called during cache population, ergo all users affected.

> Not sure what path to follow with this.

If dropping the Fixes: tag was sufficient, there remains a risk that
someone else notices the bug and fixes it. If we do treat the revert of
28339b21a365 as a "fix", can we legally tag it as fixing itself? :D

If so, I'd do that and reintroduce the feature in bug-free form.

Thanks, Phil