Re: [PATCH can] can: j1939: don't drop the last ECU reference without priv->lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can/j1939: UAF of priv->lock in j1939_ecu_timer_handler()
--

commit e395c3132226e329efed25775c10d329a0724203
Author: Patrick Dahlke <me@patrickdahlke.com>

can: j1939: don't drop the last ECU reference without priv->lock

struct j1939_ecu objects are linked into priv->ecus, which is protected
by priv->lock. __j1939_ecu_release() unlinks the ECU with list_del(), so
the final j1939_ecu_put() must happen with priv->lock held for writing.

[ ... ]

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..ec8ee0f980fb4 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -19,6 +19,11 @@ static void __j1939_ecu_release(struct kref *kref)
>  	struct j1939_ecu *ecu = container_of(kref, struct j1939_ecu, kref);
>  	struct j1939_priv *priv = ecu->priv;
>  
> +	/* priv->ecus is protected by priv->lock, so the last reference
> +	 * must only be dropped with priv->lock held for writing.
> +	 */
> +	lockdep_assert_held_write(&priv->lock);
> +
>  	list_del(&ecu->list);
>  	netdev_put(priv->ndev, &ecu->priv_dev_tracker);
>  	kfree(ecu);
>  	j1939_priv_put(priv);
>  }

[Severity: High]
Does enforcing this pattern cause a use-after-free of priv->lock?

If j1939_priv_put() drops the last reference to priv, it will free priv while
priv->lock is still held by the caller of j1939_ecu_put().

For example, the asynchronous j1939_ecu_timer_handler() executes with
write_lock_bh(&priv->lock) held, but does not hold an independent reference
to priv, relying solely on ecu->priv:

net/can/j1939/bus.c:j1939_ecu_timer_handler() {
    ...
	write_lock_bh(&priv->lock);
	...
	j1939_ecu_put(ecu);
	write_unlock_bh(&priv->lock);
    ...
}

If the network interface is down and all sockets are closed, dropping the
final ECU reference in j1939_ecu_put() invokes __j1939_ecu_release(). This
drops the ECU's reference to priv, freeing priv and priv->lock before
j1939_ecu_timer_handler() calls write_unlock_bh(&priv->lock).

Can this use-after-free be avoided by taking an explicit reference to priv
before dropping the ECU, or deferring j1939_priv_put() outside the lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-j1939-ecu-release-lock-v1-1-39dcb8a0f2f9@patrickdahlke.com?part=1