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

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

[Helen Koike]

struct nf_hook_ops has a pointer to dev, which can be used by
__nf_unregister_net_hook() after it has been freed by tun_chr_close().

Fix it  by calling dev_hold() when saving dev to ops struct.

Reported-by: syzbot+bb9127e278fa198e110c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bb9127e278fa198e110c
Signed-off-by: Helen Koike <koike@igalia.com>
---
 net/netfilter/nf_tables_api.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index fd7f7e4e2a43..00b5f900a51d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -352,6 +352,7 @@ static void nft_netdev_hook_free_ops(struct nft_hook *hook)
 
 	list_for_each_entry_safe(ops, next, &hook->ops_list, list) {
 		list_del(&ops->list);
+		dev_put(ops->dev);
 		kfree(ops);
 	}
 }
@@ -2374,6 +2375,7 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
 			err = -ENOMEM;
 			goto err_hook_free;
 		}
+		dev_hold(dev);
 		ops->dev = dev;
 		list_add_tail(&ops->list, &hook->ops_list);
 	}
-- 
2.53.0

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

[Pablo Neira Ayuso]

On Mon, Mar 02, 2026 at 06:26:05PM -0300, Helen Koike wrote:
> struct nf_hook_ops has a pointer to dev, which can be used by
> __nf_unregister_net_hook() after it has been freed by tun_chr_close().
> 
> Fix it  by calling dev_hold() when saving dev to ops struct.

Sorry, I don't think this patch works, dev_hold()/dev_put() use
per_cpu area.

The nf_tables_flowtable_event() function used to release the hook, but
now things have changed since there is auto-hook registration.

> Reported-by: syzbot+bb9127e278fa198e110c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=bb9127e278fa198e110c
> Signed-off-by: Helen Koike <koike@igalia.com>
> ---
>  net/netfilter/nf_tables_api.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index fd7f7e4e2a43..00b5f900a51d 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -352,6 +352,7 @@ static void nft_netdev_hook_free_ops(struct nft_hook *hook)
>  
>  	list_for_each_entry_safe(ops, next, &hook->ops_list, list) {
>  		list_del(&ops->list);
> +		dev_put(ops->dev);
>  		kfree(ops);
>  	}
>  }
> @@ -2374,6 +2375,7 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
>  			err = -ENOMEM;
>  			goto err_hook_free;
>  		}
> +		dev_hold(dev);
>  		ops->dev = dev;
>  		list_add_tail(&ops->list, &hook->ops_list);
>  	}
> -- 
> 2.53.0
> 

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

[Helen Koike]

Hi Pablo,

Thanks for your quick reply, please see my comments below.

On 3/2/26 8:08 PM, Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2026 at 06:26:05PM -0300, Helen Koike wrote:
>> struct nf_hook_ops has a pointer to dev, which can be used by
>> __nf_unregister_net_hook() after it has been freed by tun_chr_close().
>>
>> Fix it  by calling dev_hold() when saving dev to ops struct.
> 
> Sorry, I don't think this patch works, dev_hold()/dev_put() use
> per_cpu area.

In my understanding, being a percpu_ref shouldn't affect the behavior, 
since after a percpu_ref_kill() it switches to the usual mode with 
shared atomic_t counter.

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?

> 
> The nf_tables_flowtable_event() function used to release the hook, but
> now things have changed since there is auto-hook registration.

I'll investigate this further. Thanks a lot for the pointer (and sorry 
for the noise).

Regards,
Helen

> 
>> Reported-by: syzbot+bb9127e278fa198e110c@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=bb9127e278fa198e110c
>> Signed-off-by: Helen Koike <koike@igalia.com>
>> ---
>>   net/netfilter/nf_tables_api.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index fd7f7e4e2a43..00b5f900a51d 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -352,6 +352,7 @@ static void nft_netdev_hook_free_ops(struct nft_hook *hook)
>>   
>>   	list_for_each_entry_safe(ops, next, &hook->ops_list, list) {
>>   		list_del(&ops->list);
>> +		dev_put(ops->dev);
>>   		kfree(ops);
>>   	}
>>   }
>> @@ -2374,6 +2375,7 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
>>   			err = -ENOMEM;
>>   			goto err_hook_free;
>>   		}
>> +		dev_hold(dev);
>>   		ops->dev = dev;
>>   		list_add_tail(&ops->list, &hook->ops_list);
>>   	}
>> -- 
>> 2.53.0
>>

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

[Florian Westphal]

Helen Koike <koike@igalia.com> 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.

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

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.

		}
		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.
		case NETDEV_UNREGISTER:
			/* NOP if not found or new name still matching */
			if (!ops || (changename && match))
				continue;

			do {
				/* 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);
				ops = nft_hook_find_ops(hook, dev);
			} while (ops);
			break;

Phil, can you please have a look?  Thanks!

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@igalia.com> 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

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

[Phil Sutter]

On Tue, Mar 03, 2026 at 12:08:56AM +0100, Pablo Neira Ayuso wrote:
> On Mon, Mar 02, 2026 at 06:26:05PM -0300, Helen Koike wrote:
> > struct nf_hook_ops has a pointer to dev, which can be used by
> > __nf_unregister_net_hook() after it has been freed by tun_chr_close().
> > 
> > Fix it  by calling dev_hold() when saving dev to ops struct.
> 
> Sorry, I don't think this patch works, dev_hold()/dev_put() use
> per_cpu area.

Why is this problematic? netdev_refcnt_read() sums the per-cpu variables
up, so it should be fine if we refcount_inc on one CPU and refcount_dec
on another, no?

> The nf_tables_flowtable_event() function used to release the hook, but
> now things have changed since there is auto-hook registration.

But isn't __nf_unregister_net_hook() still called immediately when
handling NETDEV_UNREGISTER event? I guess struct nf_hook_ops::dev may
still be accessed afterwards since ops is RCU-freed. Is Helen's report
inaccurate in that regard?

Looking at netdev_wait_allrefs_any(), I see NETDEV_UNREGISTER
notification before rcu_barrier() call. Does that suffice for our
kfree_rcu() upon NETDEV_UNREGISTER?

If we really risk losing the device pointer while the holding
nf_hook_ops object is still in use, dev_hold/_put should help. But where
to call dev_put() then? AIUI, nft_netdev_hook_free_ops() does not apply
to the NETDEV_UNREGISTER event code path, does it?

Cheers, Phil

> > Reported-by: syzbot+bb9127e278fa198e110c@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=bb9127e278fa198e110c
> > Signed-off-by: Helen Koike <koike@igalia.com>
> > ---
> >  net/netfilter/nf_tables_api.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index fd7f7e4e2a43..00b5f900a51d 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -352,6 +352,7 @@ static void nft_netdev_hook_free_ops(struct nft_hook *hook)
> >  
> >  	list_for_each_entry_safe(ops, next, &hook->ops_list, list) {
> >  		list_del(&ops->list);
> > +		dev_put(ops->dev);
> >  		kfree(ops);
> >  	}
> >  }
> > @@ -2374,6 +2375,7 @@ static struct nft_hook *nft_netdev_hook_alloc(struct net *net,
> >  			err = -ENOMEM;
> >  			goto err_hook_free;
> >  		}
> > +		dev_hold(dev);
> >  		ops->dev = dev;
> >  		list_add_tail(&ops->list, &hook->ops_list);
> >  	}
> > -- 
> > 2.53.0
> > 
> 

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

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> But isn't __nf_unregister_net_hook() still called immediately when
> handling NETDEV_UNREGISTER event? I guess struct nf_hook_ops::dev may
> still be accessed afterwards since ops is RCU-freed. Is Helen's report
> inaccurate in that regard?

Its a red herring.

The device is registered twice.  But UNREGISTER only removes ONE
instance.

Then, later, when a different device (same name!) invokes netlink handler,
the walk finds the old, free'd net_device.

I hacked UNREGISTER to handle this: no more splat.
I reverted this change and altered REGISTER to never allow
double-register: no splats.

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

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > And *THIS* looks buggy.
> > Shouldn't that simply be:
> > 			if (!match || ops)
> > 				continue;

FWIW I can't get the reproducer to trigger a splat with this change.
I've fed this to syzbot to double-check.

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

Well, it does trigger, afaics.

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

[Helen Koike]

On 3/4/26 10:38 AM, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
>>> And *THIS* looks buggy.
>>> Shouldn't that simply be:
>>> 			if (!match || ops)
>>> 				continue;
> 

I tested this change locally (with syz reproducer) and I'm unable to 
trigger the issue anymore. Without this change I always reproduce it.

If you are to send this patch, please add:

Tested-by: Helen Koike <koike@igalia.com>



> FWIW I can't get the reproducer to trigger a splat with this change.
> I've fed this to syzbot to double-check.
> 
>> 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()?
> 
> Well, it does trigger, afaics.


Thanks,
Helen