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:
> > 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.

Ouch, you are right, it can be called with either attribute :-(

> > 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.

Yes, my bad.

> > 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).

Yes, I did not see that this is using different attributes.

> > 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?

Yes, right.  So no extra patch is needd.