Re: [nf-next PATCH v3 03/16] netfilter: nf_tables: Store user-defined hook ifname

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 12, 2024 at 02:56:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Prepare for hooks with NULL ops.dev pointer (due to non-existent device)
> > and store the interface name and length as specified by the user upon
> > creation. No functional change intended.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/net/netfilter/nf_tables.h | 2 ++
> >  net/netfilter/nf_tables_api.c     | 6 +++---
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index c302b396e1a7..efd6b55b4914 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -1191,6 +1191,8 @@ struct nft_hook {
> >  	struct list_head	list;
> >  	struct nf_hook_ops	ops;
> >  	struct rcu_head		rcu;
> > +	char			ifname[IFNAMSIZ];
> > +	u8			ifnamelen;
> >  };
> >  
> >  /**
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 3ffb728309af..f1710aab5188 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -2173,7 +2173,6 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
> >  					      const struct nlattr *attr)
> >  {
> >  	struct net_device *dev;
> > -	char ifname[IFNAMSIZ];
> >  	struct nft_hook *hook;
> >  	int err;
> >  
> > @@ -2183,12 +2182,13 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
> >  		goto err_hook_alloc;
> >  	}
> >  
> > -	nla_strscpy(ifname, attr, IFNAMSIZ);
> > +	nla_strscpy(hook->ifname, attr, IFNAMSIZ);
> > +	hook->ifnamelen = nla_len(attr);
> 
> 
> Hmm. nft_netdev_hook_alloc has no netlink attribute policy validation
> :-/
> 
> Can you add another patch that fixes this up?
> 
> 
> I'd suggest to move the if (nla_type(tmp) != NFTA_DEVICE_NAME)
> test from nf_tables_parse_netdev_hooks() into nft_netdev_hook_alloc
> so nft_chain_parse_netdev() also has this test.

I fear this won't work, because nft_chain_parse_netdev() may call
nft_netdev_hook_alloc() directly, passing tb[NFTA_HOOK_DEV]. For
NFTA_HOOK_DEVS though, it calls nf_tables_parse_netdev_hooks() and thus
the nested attribute type check in there applies.
> 
> Then,
> > -     nla_strscpy(ifname, attr, IFNAMSIZ);
> 
> Into:
> 
> 	err = nla_strscpy(ifname, attr, IFNAMSIZ)
> 	if (err < 0)
> 		goto err_hook_dev;
> 
> so we validate that
> a) attr is NFTA_DEVICE_NAME

As said above, it may be NFTA_HOOK_DEV as well.

> b) length doesn't exceed IFNAMSIZ.

ACK. I think a) is already being asserted in spots where NFTA_HOOK_DEV
is not expected (which in turn has a policy).

> ATM this check is implicit because nla_strscpy() always
> null-terminates and the next line will check that the
> device with this name actually exists.
> 
> But that check is removed later.
> This patch can then set
> 
>  hook->ifnamelen = err
> 
> without risk that nla_len() returns 0xfff0 or some other bogus
> value.

Ah, nice!

From my perspective, all it takes is the nla_strscpy() return value
check in this patch to cover everything. Right?

Thanks, Phil