Phil Sutter <phil@xxxxxx> 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.
