Re: [PATCH net 08/10] netfilter: nf_tables: fix device name and prefix match in hook lookup

On 8/22/26 9:52 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> netfilter: nf_tables: fix device name and prefix match in hook lookup
> 
> This changes nft_hook_list_find() so a hook spec only matches another
> spec of the same kind (prefix vs exact name) and the same length,
> replacing the previous strncmp() over min(hook->ifnamelen,
> this->ifnamelen).  hook_is_prefix() also gains a const argument.
> 
> Closes: https://lore.kernel.org/netfilter-devel/CANE+tVrDeNCHQVmsqkV2ozeBqyE3GtRDMhZgsg1bhw10yGNTRQ@mail.gmail.com/
> 
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index 20c562174b278..d160f6ea0dc29 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
> 
> [ ... ]
> 
>> @@ -2445,8 +2445,11 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
>>   	struct nft_hook *hook;
>>   
>>   	list_for_each_entry(hook, hook_list, list) {
>> -		if (!strncmp(hook->ifname, this->ifname,
>> -			     min(hook->ifnamelen, this->ifnamelen))) {
>> +		if (hook_is_prefix(hook) != hook_is_prefix(this))
>> +			continue;
>> +		if (hook->ifnamelen != this->ifnamelen)
>> +			continue;
>> +		if (!strncmp(hook->ifname, this->ifname, hook->ifnamelen)) {
>>   			if (hook->flags & NFT_HOOK_REMOVE)
>>   				continue;
>>   
> 
> [Severity: Critical]
> nft_hook_list_find() has two callers with different expectations, and this
> turns it into a pure identity comparator for both.  The delete/get paths
> (nf_tables_delchain_hook(), nft_flowtable_del_hook()) want identity, but
> the add paths use it as an overlap rejector:
> 
> nf_tables_parse_netdev_hooks()
> 	if (nft_hook_list_find(hook_list, hook)) {
> 		NL_SET_BAD_ATTR(extack, tmp);
> 		nft_netdev_hook_free(hook);
> 		err = -EEXIST;
> 		goto err_hook;
> 	}
> 
> and the same in nf_tables_updchain(), nft_register_flowtable_net_hooks()
> and nft_flowtable_update().
> 
> The Fixes-referenced commit 6d07a289504a states the min() compare was
> deliberate: "When checking for duplicate hooks, compare the shortest
> prefix so a device may never match more than a single hook spec."  Does
> this change intentionally drop that invariant?
> 
> Concretely, is this still rejected after the patch?
> 
>    nft add chain netdev t c \
>      '{ type filter hook ingress devices = { "eth*", "eth0" } ; }'
> 
> Before, prefix "eth" (ifnamelen 3) and exact "eth0" (ifnamelen 5) matched
> via strncmp(..., min(3, 5)) and gave -EEXIST.  Now
> hook_is_prefix(hook) != hook_is_prefix(this) short-circuits, so both hooks
> end up in the same hook_list.  Prefix "eth" versus prefix "eth0" is
> likewise separated by the ifnamelen compare.
> 
> nft_netdev_hook_alloc() then builds one nf_hook_ops per matching netdev
> per hook:
> 
> 	for_each_netdev(net, dev) {
> 		if (strncmp(dev->name, hook->ifname, hook->ifnamelen))
> 			continue;
> 		...
> 		ops->dev = dev;
> 		list_add_tail(&ops->list, &hook->ops_list);
> 	}
> 
> so two distinct nft_hook objects each own an ops with ops->dev == eth0,
> and nft_netdev_register_hooks() registers both.  nf_hook_entries_grow()
> only rejects duplicates for NF_HOOK_OP_BPF, so both land in
> eth0->nf_hooks_ingress and the base chain is evaluated twice per packet.
> For flowtables the cross-flowtable -EEXIST guard in
> nft_register_flowtable_net_hooks() is bypassed the same way, so the same
> device gets two FLOW_BLOCK_BIND calls.
> 
> Can this also leave a dangling struct net_device pointer?  Both notifier
> loops service only the first hook that owns an ops for the device, because
> of the unconditional break after the switch:
> 
> net/netfilter/nft_chain_filter.c:nft_netdev_event() {
> 	list_for_each_entry(hook, &basechain->hook_list, list) {
> 		ops = nft_hook_find_ops(hook, dev);
> 		...
> 		switch (event) {
> 		case NETDEV_UNREGISTER:
> 			...
> 			nf_unregister_net_hook(dev_net(dev), ops);
> 			list_del_rcu(&ops->list);
> 			kfree_rcu(ops, rcu);
> 			break;
> 		...
> 		}
> 		break;
> 	}
> 	return 0;
> }
> 
> nft_flowtable_event() in nf_tables_api.c has the same trailing break.  On
> ip link del eth0 only the first hook's ops is unregistered and freed; the
> second hook keeps a registered nf_hook_ops whose ops->dev points at the
> device that is then freed, since nf_hook_ops holds no device reference.
> 
> A later teardown (nft delete chain, nft delete table, dormant toggle,
> netns exit) walks every ops:
> 
> net/netfilter/nf_tables_api.c:nft_netdev_unregister_hooks() {
> 	list_for_each_entry_safe(hook, next, hook_list, list) {
> 		if (!(table->flags & NFT_TABLE_F_DORMANT)) {
> 			list_for_each_entry(ops, &hook->ops_list, list)
> 				nf_unregister_net_hook(net, ops);
> 		}
> 	...
> }
> 
> and __nf_unregister_net_hook() reaches nf_hook_entry_head(net, pf,
> reg->hooknum, reg->dev), which does dev_net(dev) and returns
> &dev->nf_hooks_ingress, i.e. a read and then a write through the stale
> pointer.  For flowtables, nft_unregister_flowtable_ops() additionally
> calls ->setup(..., ops->dev, FLOW_BLOCK_UNBIND) on the freed device.  The
> orphaned nf_hook_entries blob is also leaked.
> 
> On NETDEV_REGISTER the same break means only one of two covering hooks
> gains an ops, so the other stays permanently unhooked.
> 
> Would it work to keep the strict identity match only for the
> delete/update lookups and retain the prefix-overlap semantics for the
> add/-EEXIST paths, e.g. as two separate helpers?  Alternatively, should
> the unconditional break in nft_netdev_event() and nft_flowtable_event()
> be dropped so every matching hook is serviced?
> 
> The commit message describes only the delete/update lookup semantics; the
> fact that configurations previously rejected with -EEXIST are now
> accepted is not mentioned.  Could that be spelled out?
> 

Hi,

About the add path it was intended, what it wasn't is the UAF. The AI 
review managed to catch it. Btw, sashiko.dev didn't catch it, it seems 
the NIPA instance is managing to catch more subtle bugs.

This patch needs a V2 enforcing an exact match on delete/update path 
while using the current approach on the add path. I am sending one 
tomorrow first thing in the morning.

Thanks,
Fernando.