[PATCH net v4 1/5] amt: fix use-after-free when a Membership Update re-arms a freed tunnel

[Omar Ramadan <omar@blockcast.net>]

amt_update_handler() refreshes a relay tunnel's garbage-collection timer
with mod_delayed_work(&tunnel->gc_wq, ...) on a tunnel found by a lockless
list_for_each_entry_rcu() walk, holding only rcu_read_lock().  Nothing
serialises this against amt_tunnel_expire(), which runs from that same
gc_wq: once expire has taken amt->lock, done list_del_rcu() and queued
kfree_rcu(), a concurrent Update that already cached the tunnel pointer
re-arms gc_wq on an object that is about to be freed.  After the grace
period the delayed_work timer is left armed in freed memory, so it fires
into a use-after-free (and double-list_del_rcu/nr_tunnels underflow on the
way).

Make amt_tunnel_expire() the only context that frees a tunnel on the
timeout path and let it re-arm itself: the Update path only stamps
tunnel->last_seen, and expire re-reads last_seen under amt->lock to decide
whether to re-arm or free.  The Update path therefore never touches gc_wq,
so it can no longer re-queue the work of a tunnel being torn down.

amt_dev_stop() is the other freer, and it walked tunnel_list without
amt->lock while a running gc_wq could free the next entry under it.  Mark
every tunnel dead under amt->lock first, so a gc_wq that is already
running sees ->dead and bails instead of freeing in parallel; then unlink
the tunnels one at a time with list_del_rcu() under amt->lock, and cancel,
clear and free each one outside it.  The tunnel is freed exactly once, and
because readers walk tunnel_list locklessly under RCU, the entries are
unlinked in place rather than spliced onto a local list head.

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Assisted-by: LLM
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 63 +++++++++++++++++++++++++++++++++++++++++------
 include/net/amt.h | 11 +++++++++
 2 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index b53f8ec..530650d 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -1328,8 +1328,30 @@ static void amt_tunnel_expire(struct work_struct *work)
 						      struct amt_tunnel_list,
 						      gc_wq);
 	struct amt_dev *amt = tunnel->amt;
+	unsigned long deadline;
 
 	spin_lock_bh(&amt->lock);
+	/* amt_dev_stop() has taken ownership of the teardown. */
+	if (tunnel->dead) {
+		spin_unlock_bh(&amt->lock);
+		return;
+	}
+	/* A Membership Update may have refreshed the tunnel after this work
+	 * was queued.  Re-arm from last_seen rather than letting
+	 * amt_update_handler() re-arm gc_wq: this handler is then the only
+	 * context that frees the tunnel (and the only one amt_dev_stop() has
+	 * to cancel), which closes the use-after-free where a concurrent
+	 * Update re-armed gc_wq on a tunnel this function had already
+	 * list_del_rcu()'d and kfree_rcu()'d.
+	 */
+	deadline = READ_ONCE(tunnel->last_seen) +
+		   msecs_to_jiffies(amt_gmi(amt)) * 3;
+	if (time_before(jiffies, deadline)) {
+		mod_delayed_work(amt_wq, &tunnel->gc_wq, deadline - jiffies);
+		spin_unlock_bh(&amt->lock);
+		return;
+	}
+	tunnel->dead = true;
 	rcu_read_lock();
 	list_del_rcu(&tunnel->list);
 	amt->nr_tunnels--;
@@ -2494,9 +2516,13 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
 		if (tunnel->ip4 == saddr) {
 			if ((nonce == tunnel->nonce &&
 			     response_mac == tunnel->mac)) {
-				mod_delayed_work(amt_wq, &tunnel->gc_wq,
-						 msecs_to_jiffies(amt_gmi(amt))
-								  * 3);
+				/* Refresh liveness only.  gc_wq is
+				 * re-armed by amt_tunnel_expire() itself
+				 * from last_seen, never from here, so this
+				 * cannot re-queue the work of a tunnel
+				 * that is being freed.
+				 */
+				WRITE_ONCE(tunnel->last_seen, jiffies);
 				goto report;
 			} else {
 				netdev_dbg(amt->dev, "Invalid MAC\n");
@@ -2732,6 +2758,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
 	for (i = 0; i < amt->hash_buckets; i++)
 		INIT_HLIST_HEAD(&tunnel->groups[i]);
 
+	tunnel->last_seen = jiffies;
 	INIT_DELAYED_WORK(&tunnel->gc_wq, amt_tunnel_expire);
 
 	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
@@ -3050,7 +3077,7 @@ static int amt_dev_open(struct net_device *dev)
 static int amt_dev_stop(struct net_device *dev)
 {
 	struct amt_dev *amt = netdev_priv(dev);
-	struct amt_tunnel_list *tunnel, *tmp;
+	struct amt_tunnel_list *tunnel;
 	struct sk_buff *skb;
 	struct sock *sk;
 	int i;
@@ -3079,9 +3106,31 @@ static int amt_dev_stop(struct net_device *dev)
 	amt->req_cnt = 0;
 	WRITE_ONCE(amt->remote_ip, 0);
 
-	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
-		list_del_rcu(&tunnel->list);
-		amt->nr_tunnels--;
+	/* Mark every tunnel dead first, so a concurrent amt_tunnel_expire()
+	 * sees ->dead and bails instead of freeing a tunnel in parallel; this
+	 * path is then the sole freer.
+	 */
+	spin_lock_bh(&amt->lock);
+	list_for_each_entry(tunnel, &amt->tunnel_list, list)
+		tunnel->dead = true;
+	spin_unlock_bh(&amt->lock);
+
+	/* Unlink one tunnel at a time with list_del_rcu() rather than splicing
+	 * the list onto a local head: a lockless reader walking tunnel_list
+	 * must always terminate at &amt->tunnel_list, never at a stack head.
+	 */
+	for (;;) {
+		spin_lock_bh(&amt->lock);
+		tunnel = list_first_entry_or_null(&amt->tunnel_list,
+						  struct amt_tunnel_list, list);
+		if (tunnel) {
+			list_del_rcu(&tunnel->list);
+			amt->nr_tunnels--;
+		}
+		spin_unlock_bh(&amt->lock);
+		if (!tunnel)
+			break;
+
 		cancel_delayed_work_sync(&tunnel->gc_wq);
 		amt_clear_groups(tunnel);
 		kfree_rcu(tunnel, rcu);
diff --git a/include/net/amt.h b/include/net/amt.h
index 2846dde..0520121 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -240,6 +240,17 @@ struct amt_tunnel_list {
 	u32			nr_sources;
 	enum amt_status		status;
 	struct delayed_work	gc_wq;
+	/* Last time a valid Membership Update refreshed this tunnel.  gc_wq
+	 * re-arms itself from this rather than being re-armed by the Update
+	 * path, so the Update path cannot re-queue the work of a tunnel that
+	 * amt_tunnel_expire() is freeing.
+	 */
+	unsigned long		last_seen;
+	/* Set under amt->lock by whichever of amt_tunnel_expire() or
+	 * amt_dev_stop() reaches the tunnel first; the other then leaves it
+	 * alone, so the tunnel is freed exactly once.
+	 */
+	bool			dead;
 	__be16			source_port;
 	__be32			ip4;
 	__be32			nonce;