On Thu, Sep 12, 2024 at 02:56:41PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> 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@xxxxxx>
> > ---
> > 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!
