Re: [PATCH] netfilter: nf_tables: fix use-after-free on ops->dev

[Phil Sutter]

Hi,

On Wed, Mar 04, 2026 at 06:32:15AM +0100, Florian Westphal wrote:
> Helen Koike <koike@xxxxxxxxxx> wrote:
> 
> Phil, can you please take a look at this?
> 
> The reg/unregister logic is ... strange.
> 
> > But if I understood correctly from your comment below, the proper 
> > solution would be to fix the order that the hooks are released, is my 
> > understanding correct?
> 
> I don't think its about ordering.  I think the code allows to register
> devices multiple times in the same flowtable, but UNREG doesn't handle
> that.
> 
> static int nft_flowtable_event(unsigned long event, struct net_device *dev,
> 			       struct nft_flowtable *flowtable, bool changename)
> {
> 	struct nf_hook_ops *ops;
> 	struct nft_hook *hook;
> 	bool match;
> 
> 	list_for_each_entry(hook, &flowtable->hook_list, list) {
> 		ops = nft_hook_find_ops(hook, dev);
> 		match = !strncmp(hook->ifname, dev->name, hook->ifnamelen);
> 
> 		switch (event) {
> 		case NETDEV_UNREGISTER:
> 			/* NOP if not found or new name still matching */
> 			if (!ops || (changename && match))
> 				continue;
> 
> 			/* flow_offload_netdev_event() cleans up entries for us. */
> 			nft_unregister_flowtable_ops(dev_net(dev),
> 						     flowtable, ops);
> 			list_del_rcu(&ops->list);
> 			kfree_rcu(ops, rcu);
> 			break;
> 		case NETDEV_REGISTER:
> 			/* NOP if not matching or already registered */
> 			if (!match || (changename && ops))
> 				continue;
> 
> And *THIS* looks buggy.
> Shouldn't that simply be:
> 			if (!match || ops)
> 				continue;
> 
> Or can you explain why changename has any relevance here?
> changename means dev->name has already been updated.

The changename parameter is true if the event handler was called for a
NETDEV_CHANGENAME event. In that, case, we call nft_flowtable_event()
twice for each flowtable: First with NETDEV_REGISTER event value and
second with NETDEV_UNREGISTER value. If a device is renamed, we will try
to register it with all flowtables having a matching interface spec if
not already registered with. If that succeeds, we try to unregister it
from all flowtables it's registered with if not matching anymore.

In "changename mode" therefore, NETDEV_REGISTER case is skipped if name
matches but device is registered already. NETDEV_UNREGISTER case is
skipped if already registered but name still matches.

In regular mode, i.e. either real NETDEV_REGISTER or NETDEV_UNREGISTER,
the NETDEV_UNREGISTER case is skipped if the device was not registered.
If it was, the name is not relevant, we have to unregister it anyway.
The NETDEV_REGISTER case is skipped if the name does not match. If it
matches, we assume it is not registered already. This is true since
NETDEV_REGISTER notifier runs for each device just once, right?

> So, we want to skip a new registration if either:
> 1. the name doesn't match
> 2. it matches but its already registered.

The "already registered" part should not happen in real NETDEV_REGISTER
event, though: When parsing netdev hooks, non-distinct prefixes are
eliminated so that a given device name will never match more than a
single hook per flowtable. This is done by comparing with min prefix
length in nft_hook_list_find().

> In case changename is true, only UNREGISTER: case is
> relevant: If its not matching anymore -> unregister.
> 
> Still matching?  Keep it.  In that case, we havn't
> registered the device again because 'ops' was non-null in
> REGISTER case.

You're right, the 'changename' check in NETDEV_REGISTER is not needed
because even if not changing names one should skip if already
registered. Actually, this indicates a bug unless handling
NETDEV_CHANGENAME. Maybe add a WARN_ON_ONCE()?

> 		}
> 		break;
> 
> If its allowed to register the same device twice (or more), then the
> above 'break' needs to be removed, AND one has to alter UNREGISTER
> above to loop until no more ops are found, i.e.

A device may register to a single flowtable multiple times only
temporary while it is being renamed: If one hook matches the old name
and another the new name. It is supposed to register to the newly
matching hook first, then unregister from the no longer matching one.

Cheers, Phil