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 3 potential
issue(s) to consider.

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

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

- [High] In this patch's version, amt_tunnel_expire()
  (drivers/net/amt.c:1374-1383) sets tunnel->dead under tunnel->lock.
- [Medium] The commit message only describes tunnel gc_wq,
  amt_update_handler(), amt_tunnel_expire() and amt_dev_stop().
- [Medium] In this patch, amt_request_handler()
  (drivers/net/amt.c:2783-2791) sets tunnel->last_seen = jiffies and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  gnode->dead/snode->dead checks do not stop a use-after-free when a…

--

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

With this patch, amt_update_handler() only records tunnel->last_seen and no
longer re-arms gc_wq. amt_tunnel_expire() now decides from last_seen,
under amt->lock, whether to re-arm itself or free the tunnel. The patch
also changes amt_dev_stop() to mark tunnels dead under amt->lock and
unlink them one at a time, and it adds dead flags to the tunnel, group
and source nodes.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index b53f8ec556615..b06916c7d57d5 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -215,9 +215,14 @@ static struct amt_group_node *amt_lookup_group(struct amt_tunnel_list *tunnel,
>  
>  static void amt_destroy_source(struct amt_source_node *snode)
>  {
> -	struct amt_group_node *gnode = snode->gnode;
> +	struct amt_group_node *gnode;
>  	struct amt_tunnel_list *tunnel;
>  
> +	if (READ_ONCE(snode->dead))
> +		return;
> +	WRITE_ONCE(snode->dead, true);
> +	gnode = snode->gnode;
> +
>  	tunnel = gnode->tunnel_list;
>  
>  	if (!gnode->v6) {
> @@ -249,6 +254,9 @@ static void amt_del_group(struct amt_dev *amt, struct amt_group_node *gnode)
>  	struct hlist_node *t;
>  	int i;
>  
> +	if (READ_ONCE(gnode->dead))
> +		return;
> +	WRITE_ONCE(gnode->dead, true);
>  	if (cancel_delayed_work(&gnode->group_timer))
>  		dev_put(amt->dev);
>  	hlist_del_rcu(&gnode->node);

[Severity: Medium]

The commit message covers the tunnel gc_wq race and the changes to
amt_update_handler(), amt_tunnel_expire() and amt_dev_stop(). Several hunks
in the diff are outside that description:

  - a dead flag added to struct amt_source_node and struct amt_group_node
  - amt_destroy_source() and amt_del_group() made idempotent
  - amt_source_work() and amt_group_work() restructured: rcu_read_lock()
    moved ahead of the gnode reads, the RCU/spinlock nesting swapped, and
    new early exits added, one of which calls dev_put(amt->dev) when
    gnode->dead is set

Which race do the group/source changes fix? And why is the dev_put() on
the new gnode->dead exit path balanced?

The last patch in the series, "amt: harden relay authentication and
teardown", describes these same changes in its own commit message ("Mark
tunnels, groups, and sources dead under their locks; have group and source
workers hold RCU while finding their owners..."). The code itself goes in
here, though.

Could these hunks either move to that patch or be described in this commit
message?

[ ... ]

> @@ -403,26 +418,35 @@ 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_dev *amt;
>  	struct amt_source_node *snode;
>  	bool delete_group = true;
>  	struct hlist_node *t;
>  	int i, buckets;
>  
> +	rcu_read_lock();
> +	tunnel = READ_ONCE(gnode->tunnel_list);
> +	amt = READ_ONCE(gnode->amt);
>  	buckets = amt->hash_buckets;
>  
>  	spin_lock_bh(&tunnel->lock);
> +	if (READ_ONCE(gnode->dead)) {
> +		spin_unlock_bh(&tunnel->lock);
> +		rcu_read_unlock();
> +		dev_put(amt->dev);
> +		return;
> +	}

[Severity: High]

This isn't a bug introduced by this patch, but can the new gnode->dead
check actually protect amt_group_work() from amt_del_group()?

amt_del_group() does a non-sync cancel and then frees the gnode:

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

process_one_work() clears PENDING before it calls amt_group_work(), so
cancel_delayed_work() returns false and does not wait. The rcu_read_lock()
above is only taken after the callback has started. A kfree_rcu() queued
between the PENDING clear and rcu_read_lock() is therefore not held off.
With CONFIG_PREEMPT_RCU the kworker can be preempted in that window and a
grace period can complete.

In that case the callback reads gnode->tunnel_list, gnode->amt and
gnode->dead from freed memory. It then calls spin_lock_bh() through a stale
tunnel pointer and dev_put() on a stale amt->dev.

A remote gateway can reach amt_del_group() at the same time, for example
with an IGMPv2 Leave through amt_igmpv2_leave_handler() or an MLDv1 Done
sent just as group_timer fires.

amt_source_work() has the same pattern with snode->gnode and
gnode->tunnel_list. __amt_source_gc_work() also frees snodes without
calling cancel_delayed_work_sync() on snode->source_timer.

The flag lives inside the object being freed, so it can't protect the
callback that loads it. Would this need synchronous cancellation, or a
reference that covers the callback's entry?

As far as I can tell, the final patch in the series ("amt: harden relay
authentication and teardown") does not change amt_del_group(),
amt_group_work(), amt_source_work() or __amt_source_gc_work(), so the
problem is still there at the end of the series.

[ ... ]

> @@ -1328,8 +1350,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);

[ ... ]

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

[ ... ]

> @@ -2494,9 +2538,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: High]

Does this fully close the race the commit message describes? The Update
path still holds the tunnel pointer it cached from the RCU walk. After
goto report it does:

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

The amt_mld_report_handler() path is the same, and neither path checks
tunnel->dead.

last_seen is written here without amt->lock. amt_tunnel_expire() can
therefore read the old value, decide the tunnel has expired, and run:

amt_tunnel_expire()
    WRITE_ONCE(tunnel->dead, true);
    list_del_rcu(&tunnel->list);
    amt_clear_groups(tunnel);
    kfree_rcu(tunnel, rcu);

Suppose the Update takes tunnel->lock after amt_clear_groups() has released
it. Could amt_add_group() then link a new gnode into the dying tunnel and
arm group_timer with dev_hold()? For example, in
amt_igmpv2_report_handler():

    if (!mod_delayed_work(amt_wq, &gnode->group_timer,
                          msecs_to_jiffies(amt_gmi(amt))))
        dev_hold(amt->dev);

When group_timer fires later, amt_group_work() reads gnode->tunnel_list
and calls spin_lock_bh(&tunnel->lock) on the freed tunnel. The gnode and
the device reference also look leaked.

This is the same class of problem the commit message sets out to close: an
Update holding a cached tunnel pointer arms delayed work that outlives the
tunnel.

The later patch in this series, "amt: harden relay authentication and
teardown", adds a tunnel->dead check after both spin_lock_bh(&tunnel->lock)
calls in amt_update_handler(). This patch carries the Fixes: tag, so would
it make sense to move that check here?

[ ... ]

> @@ -2732,6 +2780,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);
> @@ -2739,7 +2788,7 @@ static bool amt_request_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	__amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_REQUEST, true);
>  	amt->nr_tunnels++;
>  	mod_delayed_work(amt_wq, &tunnel->gc_wq,
> -			 msecs_to_jiffies(amt_gmi(amt)));
> +			 msecs_to_jiffies(amt_gmi(amt)) * 3);
>  	spin_unlock_bh(&amt->lock);

[Severity: Medium]

Is this lifetime change intended? Before this patch, a tunnel created by a
Request had gc_wq armed for GMI. Only an authenticated Membership Update
extended it to 3*GMI.

Now the initial arm here and the re-arm in amt_tunnel_expire() both use
last_seen + 3*GMI. A tunnel that never sends an Update therefore lives
3*GMI.

Requests carry no MAC, so spoofed sources can now hold each of the
max_tunnels slots three times as long. The commit message doesn't mention
this change.

The later patch "amt: harden relay authentication and teardown" makes
amt_request_handler() stateless, so tunnels are only created after a
verified Update. That removes the problem in the final tree.

Could this patch keep the old GMI lifetime for tunnels that have never
sent an Update, or at least describe the change in the commit message?

[ ... ]

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