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

amt_group_work() dereferences gnode->tunnel_list and gnode->amt before
acquiring tunnel->lock or entering an RCU read-side critical section.
When amt_del_group() runs concurrently (e.g. from amt_clear_groups()
during interface teardown), it calls cancel_delayed_work() which cannot
stop an already-executing handler, then proceeds to kfree_rcu(gnode).
Under PREEMPT_RCU the grace period can complete while amt_group_work()
is between the container_of() and spin_lock_bh(), freeing gnode and
causing a use-after-free.

Fix by wrapping the early gnode dereferences in rcu_read_lock() to
prevent the grace period from completing while the handler accesses the
freed object. Additionally, change hlist_del_rcu() to hlist_del_init_rcu()
in amt_del_group() so amt_group_work() can detect an already-deleted gnode
via hlist_unhashed() after acquiring the lock, avoiding double-processing.

Fixes: bc54e49c140b ("amt: add multicast(IGMP) report message handler")
Signed-off-by: Zhenghang Xiao <redacted>
---
 drivers/net/amt.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index f2f3139e38a5..f937957dbd61 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c

@@ -260,7 +260,7 @@ static void amt_del_group(struct amt_dev *amt, struct amt_group_node *gnode)
 
 	if (cancel_delayed_work(&gnode->group_timer))
 		dev_put(amt->dev);
-	hlist_del_rcu(&gnode->node);
+	hlist_del_init_rcu(&gnode->node);
 	gnode->tunnel_list->nr_groups--;
 
 	if (!gnode->v6)

@@ -412,23 +412,31 @@ static void amt_group_work(struct work_struct *work)
 	struct amt_group_node *gnode = container_of(to_delayed_work(work),
 						    struct amt_group_node,
 						    group_timer);
-	struct amt_tunnel_list *tunnel = gnode->tunnel_list;
-	struct amt_dev *amt = gnode->amt;
+	struct amt_tunnel_list *tunnel;
 	struct amt_source_node *snode;
 	bool delete_group = true;
+	struct amt_dev *amt;
 	struct hlist_node *t;
 	int i, buckets;
 
+	rcu_read_lock();
+	tunnel = gnode->tunnel_list;
+	amt = gnode->amt;
 	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) {

-- 
2.50.1 (Apple Git-155)

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.