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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=bb9127e278fa198e110c
> > Signed-off-by: Helen Koike <koike@xxxxxxxxxx>
> > ---
> >  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
> > 
>