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

[Florian Westphal <fw@strlen.de>]

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.

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
b) length doesn't exceed IFNAMSIZ.

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.