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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] amt: workqueue deadlock in amt_tunnel_expire() and
  __amt_group_gc_work()
- [Medium] amt: jiffies underflow in amt_tunnel_expire() when re-arming
  gc_wq
--

commit 96e3ef58f0da0bec1417666e7bc125f374740408
Author: Omar Ramadan <omar@blockcast.net>

amt: fix use-after-free when a Membership Update re-arms a freed tunnel

This commit fixes a use-after-free where amt_update_handler() could re-arm
gc_wq on a tunnel concurrently being freed by amt_tunnel_expire(), by
tracking tunnel liveness via last_seen and deferring re-arming to
amt_tunnel_expire(). It also updates tunnel, group, and source teardown to
mark objects dead under tunnel->lock and defer freeing via RCU and dedicated
garbage-collection work items.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 81d26ef82cf5c..d8914d811f928 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
[ ... ]
> @@ -106,6 +111,42 @@ static void amt_source_gc_work(struct work_struct *work)
>  	spin_unlock_bh(&source_gc_lock);
>  }
>  
> +static void __amt_group_gc_work(void)
> +{
> +	struct amt_group_node *gnode;
> +	struct hlist_head gc_list;
> +	struct hlist_node *t;
> +
> +	spin_lock_bh(&group_gc_lock);
> +	hlist_move_list(&group_gc_list, &gc_list);
> +	spin_unlock_bh(&group_gc_lock);
> +
> +	/* Group deletion queues all of its sources first.  Flush that queue
> +	 * so they are reclaimed promptly; a source callback cannot reach a
> +	 * freed group because it resolves snode->gnode under RCU and the
> +	 * pointer is cleared before the source is queued.  Flush rather than
> +	 * cancel: source_gc_wq re-arms only itself, so cancelling it here
> +	 * would stop periodic source GC for good.
> +	 */
> +	flush_delayed_work(&source_gc_wq);
[ ... ]
> @@ -1342,14 +1415,40 @@ 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 (READ_ONCE(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 on the timeout path; amt_dev_stop()
> +	 * still owns teardown during device shutdown.
> +	 */
> +	deadline = READ_ONCE(tunnel->last_seen);
> +	if (!deadline)
> +		deadline = jiffies;
> +	else
> +		deadline += msecs_to_jiffies(amt_gmi(amt)) * 3;
> +	if (time_before(jiffies, deadline)) {
> +		mod_delayed_work(amt_wq, &tunnel->gc_wq, deadline - jiffies);

[Severity: Medium]
Can jiffies advance past deadline between the time_before(jiffies, deadline)
check and the subtraction in mod_delayed_work() in amt_tunnel_expire()?

Since jiffies is volatile and spin_lock_bh(&amt->lock) only disables softirqs
while leaving timer interrupts enabled, reading jiffies twice without
snapshotting it into a local variable allows jiffies to tick past deadline
between the two reads. If that happens, deadline - jiffies underflows to a
value near ULONG_MAX, scheduling tunnel->gc_wq far in the future so the
tunnel is never expired until device teardown.

Would snapshotting unsigned long now = jiffies once before computing and
comparing against deadline avoid this underflow?

> +		spin_unlock_bh(&amt->lock);
> +		return;
> +	}
> +	spin_lock_bh(&tunnel->lock);
> +	WRITE_ONCE(tunnel->dead, true);
> +	spin_unlock_bh(&tunnel->lock);
>  	rcu_read_lock();
>  	list_del_rcu(&tunnel->list);
>  	amt->nr_tunnels--;
>  	amt_clear_groups(tunnel);
>  	rcu_read_unlock();
>  	spin_unlock_bh(&amt->lock);
> +	flush_delayed_work(&group_gc_wq);

[Severity: High]
Can calling flush_delayed_work(&group_gc_wq) from amt_tunnel_expire() and
flush_delayed_work(&source_gc_wq) from amt_group_gc_work() ->
__amt_group_gc_work() deadlock amt_wq?

In amt_init(), amt_wq is created as a single shared unbound workqueue:

    amt_wq = alloc_workqueue("amt", WQ_UNBOUND, 0);

which has a finite max_active limit (and per-node active limits) shared by
tunnel->gc_wq, group_gc_wq, source_gc_wq, gnode->group_timer, and
snode->source_timer.

Both group_gc_wq and source_gc_wq normally sit on their 30-second periodic
timers and do not hold an active workqueue slot. When flush_delayed_work()
runs from a worker already executing on amt_wq, it deletes the timer and calls
__queue_work() to queue the target work item onto amt_wq, then blocks in
flush_work() -> wait_for_completion() while still holding its own active slot
on amt_wq.

If all active slots on amt_wq are occupied by concurrent amt_tunnel_expire()
or timer work items (or if pwq->inactive_works is non-empty), the newly
queued group_gc_wq or source_gc_wq is placed on pwq->inactive_works and
cannot run until an active work item finishes, while the active workers are
blocked waiting for that inactive GC work item to complete. This will also
stall amt_dev_stop() under RTNL when it waits on cancel_delayed_work_sync()
or flush_delayed_work().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105125010.1107546-1-omar@blockcast.net?part=1