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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 1 · High: 0 (1) · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] At this commit the Update-vs-teardown race is only partly
  closed.
- [Medium] At this commit the initial gc_wq arm and the new expiry
  deadline disagree.
- [Medium] At this commit amt_dev_stop() sets ->dead on all tunnels in
  one amt->lock section and unlinks them later in separate amt->lock…
- [Low] The new comments disagree about which code frees a tunnel.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but tearing down a
  group does not wait for a group_timer callback that is already running.

--

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

This patch stops amt_update_handler() from re-arming a relay tunnel's
gc_wq. The Update path now only stamps tunnel->last_seen, and
amt_tunnel_expire() uses last_seen under amt->lock to decide whether to
re-arm itself or free the tunnel. It also adds a dead flag and changes
amt_dev_stop() to mark every tunnel dead under amt->lock, then unlink and
free them one at a time.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index b53f8ec556615..530650d5c19e9 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1328,8 +1328,30 @@ static void amt_tunnel_expire(struct work_struct *work)

[ ... ]

>  	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.
> +	 */

[Severity: Low]
This isn't a bug, but the comments in this patch don't agree on which
code frees a tunnel. This one says amt_tunnel_expire() is "the only
context that frees the tunnel". The new comment in amt_dev_stop() says
"this path is then the sole freer", and amt_dev_stop() does call
kfree_rcu() on every tunnel left on the list.

The dead field comment in include/net/amt.h says either function may free
the tunnel, whichever gets there first. The commit message says "the only
context that frees a tunnel on the timeout path".

Could this comment keep the "on the timeout path" qualifier? Both comments
are still unchanged at the end of the series.

[ ... ]

> @@ -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;

[Severity: Critical]
Can this cached tunnel pointer still be used after teardown? The tunnel
comes from a lockless list_for_each_entry_rcu() walk. After goto report,
amt_update_handler() does this without checking tunnel->dead:

    spin_lock_bh(&tunnel->lock);
    amt_igmp_report_handler(amt, skb, tunnel);
    spin_unlock_bh(&tunnel->lock);

If amt_tunnel_expire() or amt_dev_stop() runs at the same time:

CPU1 (amt_tunnel_expire)
  tunnel->dead = true;
  list_del_rcu(&tunnel->list);
  amt_clear_groups(tunnel);   /* takes and drops tunnel->lock */
  kfree_rcu(tunnel, rcu);

CPU2 (amt_update_handler)
  spin_lock_bh(&tunnel->lock);
  amt_igmp_report_handler()
    amt_igmpv2_report_handler()
      amt_add_group()
        gnode->tunnel_list = tunnel;
        hlist_add_head_rcu(&gnode->node, &tunnel->groups[hash]);

Nothing ever clears the new gnode, and its group_timer is armed with
dev_hold(). After the grace period the tunnel is freed, and
amt_group_work() later does this on freed memory:

    struct amt_tunnel_list *tunnel = gnode->tunnel_list;
    ...
    spin_lock_bh(&tunnel->lock);

The gnode and the netdev reference also leak. This is the same
cached-pointer pattern the commit message describes. The MLD path
(amt_mld_report_handler()) has the same gap.

The later patch "amt: harden relay tunnel lifetime and request admission"
adds a READ_ONCE(tunnel->dead) check under tunnel->lock before both report
handlers. That closes this window, but the check is not present at this
commit.

Even with that check, an Update that stamps last_seen just after
amt_tunnel_expire() reads it at the 3 * GMI boundary loses that one
refresh. This only drops the report and does not cause a memory-safety
problem.

[ ... ]

> @@ -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);

[Severity: Medium]
amt_request_handler() still arms gc_wq for a single GMI a few lines below:

    mod_delayed_work(amt_wq, &tunnel->gc_wq,
                     msecs_to_jiffies(amt_gmi(amt)));

amt_tunnel_expire() now uses a 3 * GMI deadline:

    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);

When the work first fires at 1 * GMI, time_before() is true, so the work
re-arms for about 2 * GMI more instead of freeing the tunnel.

Does a tunnel created by a Request, and never confirmed by a MAC-valid
Membership Update, now live for 3 * GMI instead of 1 * GMI? That would
hold its max_tunnels slot three times longer than before.

Also, the new last_seen comment in include/net/amt.h says "Last time a
valid Membership Update refreshed this tunnel". At this commit the Request
path sets it without any Update.

The later patch "amt: harden relay tunnel lifetime and request admission"
makes amt_request_handler() stateless. It creates tunnels only in
amt_tunnel_get_or_create(), which arms gc_wq for 3 * GMI, so the arm time
and the deadline match by the end of the series. They don't match at this
commit.

[ ... ]

> @@ -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);

[Severity: Medium]
Can a new tunnel be added between this marking pass and the unlink loop
below? The unlink loop takes amt->lock again for each entry and never sets
->dead itself.

The RX path doesn't seem to be quiesced at this point. Earlier in
amt_dev_stop():

    RCU_INIT_POINTER(amt->sk, NULL);
    synchronize_net();
    if (sk)
        udp_tunnel_sock_release(sk);

amt_rcv() finds the device through rcu_dereference_sk_user_data(sk).
udp_tunnel_sock_release() clears sk_user_data after synchronize_net() and
does not wait for another grace period. amt_request_handler() also has no
netif_running() check before list_add_tail_rcu() and arming gc_wq.

So a Request still in flight could publish a tunnel with dead == false and
gc_wq armed after this pass. That tunnel could stay on tunnel_list after
stop.

If its gc_wq instead runs between the list_del_rcu() below and
cancel_delayed_work_sync(), amt_tunnel_expire() sees dead == false. Both
contexts would then unlink and kfree_rcu() the same tunnel. That needs
gc_wq, armed for at least one GMI, to fire inside a very short window, so
it is unlikely.

The later patch "amt: harden relay tunnel lifetime and request admission"
moves tunnel creation into amt_tunnel_get_or_create(). That function checks
!netif_running(amt->dev) under amt->lock, which closes this by the end of
the series.

[ ... ]

> +	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);

[Severity: High]
This isn't a bug introduced by this patch, but does group teardown wait
for a group_timer callback that is already running? amt_del_group() does:

    if (cancel_delayed_work(&gnode->group_timer))
        dev_put(amt->dev);
    hlist_del_rcu(&gnode->node);
    gnode->tunnel_list->nr_groups--;
    ...
    kfree_rcu(gnode, rcu);

cancel_delayed_work() returns false once amt_group_work() has started.
amt_group_work() reads gnode->tunnel_list and amt->hash_buckets before it
takes tunnel->lock. It never checks whether the gnode was already deleted.

Take an IGMPv2 group (MCAST_EXCLUDE, group_timer armed at GMI). If a
remote IGMPv2 Leave, or amt_clear_groups(), gets tunnel->lock while that
timer is running:

CPU1
amt_igmpv2_leave_handler()
  amt_del_group(amt, gnode)
    hlist_del_rcu(&gnode->node);
    kfree_rcu(gnode, rcu);

CPU2
amt_group_work()
  spin_lock_bh(&tunnel->lock);
  /* still MCAST_EXCLUDE, no sources, delete_group = true */
  amt_del_group(amt, gnode)
    hlist_del_rcu(&gnode->node);   /* pprev is LIST_POISON2 */

Would the second hlist_del_rcu() oops here? This path also decrements
nr_groups a second time and calls kfree_rcu() a second time.

On preemptible kernels, amt_group_work() can also be preempted before
spin_lock_bh(). The gnode's kfree_rcu() does not wait for it. Neither does
the tunnel's kfree_rcu() here, which follows amt_clear_groups() in
amt_dev_stop() or amt_tunnel_expire(). On resume, the work could touch
freed gnode and tunnel memory, including spin_lock_bh() on a freed
tunnel->lock. The dev_hold() taken when group_timer is armed only keeps
the netdev alive.

amt_dev_stop() only calls cancel_delayed_work_sync() on gc_wq, not on the
group or source timers. This code is unchanged through the end of the
series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013175407.947606-1-omar%40blockcast.net