Re: [PATCH net] amt: fix use-after-free in amt_group_work

From: AI Reviewer <redacted>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
amt: fix use-after-free in amt_group_work

This patch wraps the early gnode dereferences in amt_group_work() with
rcu_read_lock() and switches amt_del_group() to hlist_del_init_rcu() so
the worker can detect an already-deleted gnode via hlist_unhashed().
The stated goal is to close a use-after-free window between the
container_of() in the worker and the spin_lock_bh() acquisition.

[ ... ]

[Medium]
Does this fully close the race the commit message describes?

The worker entry from the workqueue core through container_of() and the
local variable declarations runs before rcu_read_lock() executes.  If
amt_group_work() is preempted in that window (permitted under
PREEMPT_RCU and PREEMPT_RT, since a task that is not currently in a
read-side critical section is a quiescent state for that task), and on
another CPU amt_del_group() runs from amt_clear_groups() and the GP it
was waiting on completes:

- the GP only needs to wait for readers active when call_rcu()/kfree_rcu()
  was invoked, not for the preempted worker
- once the caller of amt_clear_groups() drops its rcu_read_lock(), the
  GP can complete and gnode is freed
- the preempted worker resumes and calls rcu_read_lock() as a new
  reader, which does not retroactively extend the already-completed
  grace period

When the worker resumes, the very first line:

	tunnel = gnode->tunnel_list;

reads from freed memory, and the subsequent hlist_unhashed(&gnode->node)
check operates on a freed hlist_node whose ->pprev may hold any value.

Would cancel_delayed_work_sync() in the destruction path (with care for
the self-cancellation case where amt_group_work() itself calls
amt_del_group()), or taking an explicit reference on gnode at queue
time, be a more robust fix?

 	buckets = amt->hash_buckets;
 
 	spin_lock_bh(&tunnel->lock);
+	if (hlist_unhashed(&gnode->node)) {
+		spin_unlock_bh(&tunnel->lock);
+		rcu_read_unlock();
+		goto out;
+	}
 	if (gnode->filter_mode == MCAST_INCLUDE) {
 		/* Not Used */
 		spin_unlock_bh(&tunnel->lock);
+		rcu_read_unlock();
 		goto out;
 	}
 
-	rcu_read_lock();
 	for (i = 0; i < buckets; i++) {
 		hlist_for_each_entry_safe(snode, t,
 					  &gnode->sources[i], node) {

[High]
Does the same UAF class still apply to amt_source_work() in the same
file?  It performs the early dereferences before taking either
tunnel->lock or rcu_read_lock():

static void amt_source_work(struct work_struct *work)
{
	struct amt_source_node *snode = container_of(to_delayed_work(work),
						     struct amt_source_node,
						     source_timer);
	struct amt_group_node *gnode = snode->gnode;
	struct amt_dev *amt = gnode->amt;
	struct amt_tunnel_list *tunnel;

	tunnel = gnode->tunnel_list;
	spin_lock_bh(&tunnel->lock);
	rcu_read_lock();
	...
}

The destruction path amt_destroy_source() uses cancel_delayed_work()
(non-sync) and the deferred __amt_source_gc_work() ultimately
kfree_rcu()s snode.  If amt_source_work() is preempted between
container_of() and the spin_lock_bh()/rcu_read_lock(), while a
concurrent destroy of snode runs and completes its grace period, the
snode->gnode and gnode->tunnel_list reads here become use-after-free of
snode (and potentially gnode).

This isn't introduced by this patch, but should the same fix be applied
to amt_source_work() as part of this series, given it is the same bug
class in the same driver?
-- 
This is an AI-generated review.