Re: [PATCH net 02/10] netfilter: nfnetlink_queue: hold nfnl mutex in event notifier

Jakub Kicinski <[email protected]> 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: nfnetlink_queue: hold nfnl mutex in event notifier
> 
> This takes the NFNL_SUBSYS_QUEUE mutex around the instance destroy loop in
> nfqnl_rcv_nl_event() so that the NETLINK_URELEASE notifier can no longer run
> in parallel with nfqnl_recv_config().  The stale comment in nfqnl_recv_config()
> is updated to describe the new serialization.

This fixes a unpriv netns root exploit.  At least I think so.  Its
simple enough to look at and should be small enough to backport.

The LLM report makes me wonder:

1. Does this patch not fix it?  If so, how does the reproducer need
to be changed to make it trigger KASAN/UaF again?  Doesn't say.

2. Does this add a regression?  Doesn't say.  Tests I have pass.

> > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> > index c727668b0c5be..a3bc002800510 100644
> > --- a/net/netfilter/nfnetlink_queue.c
> > +++ b/net/netfilter/nfnetlink_queue.c
> > @@ -1593,6 +1593,7 @@ nfqnl_rcv_nl_event(struct notifier_block *this,
> >  	if (event == NETLINK_URELEASE && n->protocol == NETLINK_NETFILTER) {
> >  		int i;
> >  
> > +		nfnl_lock(NFNL_SUBSYS_QUEUE);
> >  		/* destroy all instances for this portid */
> >  		spin_lock(&q->instances_lock);
> >  		for (i = 0; i < INSTANCE_BUCKETS; i++) {
> 
> [Severity: Medium]
> This is a pre-existing issue, not something introduced here, but does waiting
> on the nfnl queue mutex here make it easier to destroy an instance that belongs
> to a different socket?

So what?

This is a 20 year old interface. NOONE, and I really mean, NO ONE, has
raised a bug report in all these years.

*WHAT* does a *SENSIBLE* userspace program need to do so that it ends up
in a bad state?

From where I'm standing, there is a crash bug that is fixed by
this patch without observable new regression.

Aside from not crashing, kernel should also not have to perform absurd
stunts to somehow make sense of whatever crap userspace is trying to do.

> The destroy loop in nfqnl_rcv_nl_event() picks victims by portid only:
> 
> 	hlist_for_each_entry_safe(inst, t2, head, hlist) {
> 		if (n->portid == inst->peer_portid)
> 			__instance_destroy(inst);
> 	}
> 
> and net/netlink/af_netlink.c:netlink_release() makes the portid reusable well
> before the notifier runs:
> 
> netlink_release()
> 	netlink_remove(sk);		/* portid is free for reuse here */
> 	sock_orphan(sk);
> 	...
> 	blocking_notifier_call_chain(&netlink_chain,
> 				     NETLINK_URELEASE, &n);
> 
> With the new nfnl_lock(NFNL_SUBSYS_QUEUE), the notifier now blocks for the
> whole duration of any NFQNL_MSG_CONFIG handling (including instance_create()
> and its sleeping rhashtable_init()), in any netns, since
> net/netfilter/nfnetlink.c:nfnl_lock() takes the single global
> table[NFNL_SUBSYS_QUEUE].mutex.  So this sequence looks possible:
> 
> thread 1: close() of a NETLINK_NETFILTER socket with portid X
> 	  netlink_remove() releases X, notifier is pending

This is normally done by some applications when they shut down as
part of SIGTERM for example. Others just exit().

> thread 2: binds portid X, sends NFQNL_CFG_CMD_BIND
> 	  nfnetlink_rcv_msg() -> nfqnl_recv_config() under the queue mutex
> 	  instance_create() links a fresh instance with peer_portid == X

Why would one try do that...?  Just... don't do that?
And I don't follow how this can work.  LLM clains thread 1 is now
blocked in nfnl_lock(NFNL_SUBSYS_QUEUE), which thread 2 holds.

Therefore, as thread 1 couldn't yet run the notifier, the old queue is
still registered... no?

> thread 1: notifier finally acquires the mutex, matches
> 	  inst->peer_portid == X and calls __instance_destroy() on the
> 	  brand new instance

> Can the ownership check that the new comment relies on detect this?
> 
> 	queue = instance_lookup(q, queue_num);
> 	if (queue && queue->peer_portid != NETLINK_CB(skb).portid) {
> 
> peer_portid matches in the recycled-portid case, so the check passes and the
> instance is still torn down by the older socket's notification.

Not following :-/

Even if that test passes, why does it not hit the 'return -EBUSY' in
NFQNL_CFG_CMD_BIND?

If it did not, then instance_lookup() did not return a result, i.e.
queue was already done?  Can't make sense of anything anymore.

> There is no memory safety problem, the instance is unlinked once and freed via
> queue_rcu_work().  The visible effect is that a live socket's queue silently
> disappears, its queued packets are flushed, packets hitting the NFQUEUE rule
> then fail with -ESRCH (dropped unless fail-open or bypass is set), and later
> CONFIG messages for that queue number return -ENODEV.  It requires
> CAP_NET_ADMIN in the netns and a concurrent close/bind, and no later commit in
> this series touches net/netfilter/nfnetlink_queue.c.

I have no idea how to sensibly change this patch to address this 'bug'.

And thats my main source of frustration with all the LLM walls we have
now in place.

A human telling me that I fucked up usually has the courtesy of providing
an alternative solution or a hint to get things move in the right
direction again.

Or even 'I don't like it because of problem XYZ but I don't have a better
idea either'.

LLM just says 'something is wrong, you figure out the details of fixing
this'.  I would like to make better patches, but I don't know how.