Re: [PATCH v7 2/2] can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: duplicate ECU creation and UAF when j1939_ecu_get() fails
--

commit 85d70b39e15375400811d45130221248f1712952
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

can: j1939: fix "struct j1939_ecu" refcount leak in j1939_ecu_map_locked()

This commit changes j1939_ecu_get() to fail when the associated net_device is
no longer in NETREG_REGISTERED state or when priv->rx_kref is zero, preventing
j1939_ecu_map_locked() from remapping an ECU after j1939_ecu_unmap_all() has
completed.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..11d08b7485489 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -50,8 +50,13 @@ void j1939_ecu_put(struct j1939_ecu *ecu)
>  
>  static __must_check bool j1939_ecu_get(struct j1939_ecu *ecu)
>  {
> -	/* Don't get an ECU ref if __j1939_ecu_release() was called. */
> -	return kref_get_unless_zero(&ecu->kref);
> +	struct j1939_priv *priv = ecu->priv;
> +
> +	/* Don't get an ECU ref if netdev is no longer registered or
> +	 * __j1939_rx_release() was called or __j1939_ecu_release() was called.
> +	 */
> +	return READ_ONCE(priv->ndev->reg_state) == NETREG_REGISTERED &&
> +		kref_read(&priv->rx_kref) && kref_get_unless_zero(&ecu->kref);
>  }

[Severity: Critical]
Can this cause duplicate struct j1939_ecu creation, use-after-free when
ecu->ac_timer fires, and a leaked net_device reference when
j1939_ecu_get() rejects a live ECU?

With this change, j1939_ecu_get() returns false whenever
priv->ndev->reg_state != NETREG_REGISTERED or kref_read(&priv->rx_kref) == 0,
even if an existing ECU on priv->ecus still has ecu->kref > 0 (for example,
while its 250ms ac_timer is pending or while another socket holds it).

However, both callers of j1939_ecu_get_by_name_locked() that manage named
ECUs assume a NULL return means no valid ECU with that name exists, and fall
back to j1939_ecu_create_locked():

net/can/j1939/bus.c:j1939_local_ecu_get() {
	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (!ecu)
		ecu = j1939_ecu_create_locked(priv, name);
	if (IS_ERR(ecu))
		goto unlock_priv;
}

net/can/j1939/address-claim.c:j1939_ac_process() {
	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (ecu && ecu->addr == skcb->addr.sa) {
		...
	}
	if (!ecu && j1939_address_is_unicast(skcb->addr.sa))
		ecu = j1939_ecu_create_locked(priv, name);
}

Because j1939_ecu_create_locked() does not check priv->ndev->reg_state or
priv->rx_kref, a concurrent j1939_sk_bind() -> j1939_local_ecu_get() or
in-flight j1939_can_recv() -> j1939_ac_process() running after reg_state
transitions to NETREG_UNREGISTERING (or after rx_kref reaches 0) will skip
the existing ECU on priv->ecus and append a second struct j1939_ecu with the
same name to priv->ecus.

When the socket later unbinds in j1939_sk_netdev_event_unregister() or
j1939_sk_release(), j1939_local_ecu_put() looks up the ECU via
j1939_ecu_find_by_name_locked(), which checks j1939_ecu_valid():

net/can/j1939/bus.c:j1939_ecu_valid() {
	/* Treat an ECU as invalid if __j1939_ecu_release() was called. */
	return kref_read(&ecu->kref);
}

net/can/j1939/bus.c:j1939_local_ecu_put() {
	ecu = j1939_ecu_find_by_name_locked(priv, name);
	if (WARN_ON_ONCE(!ecu))
		goto done;

	ecu->nusers--;
	/* TODO: do we care if ecu->addr != sa? */
	if (j1939_ecu_is_mapped_locked(ecu))
		/* ecu's sa is active already */
		priv->ents[ecu->addr].nusers--;
	j1939_ecu_put(ecu);
}

Because j1939_ecu_valid() only checks ecu->kref, j1939_local_ecu_put()
matches the first ECU on priv->ecus instead of the newly created duplicate.
This drops the first ECU's reference (which may belong to its pending
ac_timer, freeing the first ECU while ac_timer is still armed) and leaves the
second ECU at the tail of priv->ecus permanently leaked along with its
struct j1939_priv and net_device references.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/00aae7aa-8351-4d69-8938-4d8e4eee78c2@I-love.SAKURA.ne.jp?part=2