[PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[Pablo Neira Ayuso]

Validate table family when looking up for it via NFTA_TABLE_HANDLE.

Reported-by: Xingyuan Mo <hdthky0@gmail.com>
Fixes: 3ecbfd65f50e ("netfilter: nf_tables: allocate handle and delete objects via handle")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c0a42989b982..c5c17c6e80ed 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -803,7 +803,7 @@ static struct nft_table *nft_table_lookup(const struct net *net,
 
 static struct nft_table *nft_table_lookup_byhandle(const struct net *net,
 						   const struct nlattr *nla,
-						   u8 genmask, u32 nlpid)
+						   int family, u8 genmask, u32 nlpid)
 {
 	struct nftables_pernet *nft_net;
 	struct nft_table *table;
@@ -811,6 +811,7 @@ static struct nft_table *nft_table_lookup_byhandle(const struct net *net,
 	nft_net = nft_pernet(net);
 	list_for_each_entry(table, &nft_net->tables, list) {
 		if (be64_to_cpu(nla_get_be64(nla)) == table->handle &&
+		    table->family == family &&
 		    nft_active_genmask(table, genmask)) {
 			if (nft_table_has_owner(table) &&
 			    nlpid && table->nlpid != nlpid)
@@ -1544,7 +1545,7 @@ static int nf_tables_deltable(struct sk_buff *skb, const struct nfnl_info *info,
 
 	if (nla[NFTA_TABLE_HANDLE]) {
 		attr = nla[NFTA_TABLE_HANDLE];
-		table = nft_table_lookup_byhandle(net, attr, genmask,
+		table = nft_table_lookup_byhandle(net, attr, family, genmask,
 						  NETLINK_CB(skb).portid);
 	} else {
 		attr = nla[NFTA_TABLE_NAME];
-- 
2.30.2

Re: [PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Validate table family when looking up for it via NFTA_TABLE_HANDLE.
> 
> Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> Fixes: 3ecbfd65f50e ("netfilter: nf_tables: allocate handle and delete objects via handle")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_tables_api.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

This changes behaviour, before this change you can do

nft delete table handle 42

and it will delete the table with handle 42.

After this change, the command will only work if this table happens
to be in 'ip' family.

> -		table = nft_table_lookup_byhandle(net, attr, genmask,
> +		table = nft_table_lookup_byhandle(net, attr, family, genmask,
>  						  NETLINK_CB(skb).portid);

Perhaps leave as-is and:
	if (!IS_ERR(table))
		family = table->family?

(or ctx.family =, but then the strange ctx.family assignment at end
 of function needs to go).

Re: [PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[Pablo Neira Ayuso]

On Mon, Dec 04, 2023 at 03:03:41PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Validate table family when looking up for it via NFTA_TABLE_HANDLE.
> > 
> > Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> > Fixes: 3ecbfd65f50e ("netfilter: nf_tables: allocate handle and delete objects via handle")
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_tables_api.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> This changes behaviour, before this change you can do
> 
> nft delete table handle 42
> 
> and it will delete the table with handle 42.

Default family is 'ip' if not specified, that is inconsistent with
other objects?

> After this change, the command will only work if this table happens
> to be in 'ip' family.
> 
> > -		table = nft_table_lookup_byhandle(net, attr, genmask,
> > +		table = nft_table_lookup_byhandle(net, attr, family, genmask,
> >  						  NETLINK_CB(skb).portid);
> 
> Perhaps leave as-is and:
> 	if (!IS_ERR(table))
> 		family = table->family?
> 
> (or ctx.family =, but then the strange ctx.family assignment at end
>  of function needs to go).

Re: [PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[Phil Sutter]

On Mon, Dec 04, 2023 at 03:05:45PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Dec 04, 2023 at 03:03:41PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Validate table family when looking up for it via NFTA_TABLE_HANDLE.
> > > 
> > > Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> > > Fixes: 3ecbfd65f50e ("netfilter: nf_tables: allocate handle and delete objects via handle")
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > ---
> > >  net/netfilter/nf_tables_api.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > This changes behaviour, before this change you can do
> > 
> > nft delete table handle 42
> > 
> > and it will delete the table with handle 42.
> 
> Default family is 'ip' if not specified, that is inconsistent with
> other objects?

I would say the table's handle is a complete replacement of its family
and name, also because it's pernet-unique. Though looking at the docs,
we surprisingly claim to support:

| delete table [<family>] handle <handle>

So either we accept user space is wrong and the family value doesn't
matter there or we artificially limit table lookup by handle to the
given family. IMHO either way kind of breaks user space.

Off-topic here, but I would prefer for all handles to be pernet-unique
so user space could 'nft delete <whatever> handle <handle>'. Why was a
table-unique value chosen here?

Cheers, Phil

Re: [PATCH nf] netfilter: nf_tables: validate family when identifying table via handle

[Pablo Neira Ayuso]

On Tue, Dec 05, 2023 at 02:10:40PM +0100, Phil Sutter wrote:
> On Mon, Dec 04, 2023 at 03:05:45PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Dec 04, 2023 at 03:03:41PM +0100, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > Validate table family when looking up for it via NFTA_TABLE_HANDLE.
> > > > 
> > > > Reported-by: Xingyuan Mo <hdthky0@gmail.com>
> > > > Fixes: 3ecbfd65f50e ("netfilter: nf_tables: allocate handle and delete objects via handle")
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > ---
> > > >  net/netfilter/nf_tables_api.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > This changes behaviour, before this change you can do
> > > 
> > > nft delete table handle 42
> > > 
> > > and it will delete the table with handle 42.
> > 
> > Default family is 'ip' if not specified, that is inconsistent with
> > other objects?
> 
> I would say the table's handle is a complete replacement of its family
> and name, also because it's pernet-unique.

Only tables and newer kernels >= 5.15 -stable, yes.

> Though looking at the docs, we surprisingly claim to support:
> 
> | delete table [<family>] handle <handle>

For consistency with existing objects, yes.

> So either we accept user space is wrong and the family value doesn't
> matter there or we artificially limit table lookup by handle to the
> given family. IMHO either way kind of breaks user space.
>
> Off-topic here, but I would prefer for all handles to be pernet-unique
> so user space could 'nft delete <whatever> handle <handle>'. Why was a
> table-unique value chosen here?