Re: [PATCH v8 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 2 potential issue(s) to consider:
- [High] can: j1939: use-after-free in j1939_ecu_timer_handler() on module exit
- [High] can: j1939: missing reg_state check in j1939_local_ecu_get() on bind
--

commit 4a42bc98a7b7f2c36e99bd97ffdcb80e028003ad
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 fixes a struct j1939_ecu reference count leak when
j1939_ecu_timer_handler() maps an ECU after __j1939_rx_release() has already
unmapped all ECUs. It drops the ECU reference in j1939_ecu_map_locked() and
rejects ECU creation in j1939_ecu_create_locked() if the net_device is no
longer registered or priv->rx_kref is zero.

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index 3422e31338004..77b6788060573 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -84,6 +84,15 @@ static void j1939_ecu_map_locked(struct j1939_ecu *ecu)
>  	if (!j1939_ecu_get(ecu))
>  		return;
>  
> +	/* Drop the ref if netdev is no longer registered or
> +	 * __j1939_rx_release() was called.
> +	 */
> +	if (READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED ||
> +	    !kref_read(&priv->rx_kref)) {
> +		j1939_ecu_put(ecu);
> +		return;
> +	}

[Severity: High]
Because ecu->ac_timer is not canceled during __j1939_rx_release() ->
j1939_ecu_unmap_all(), can this cause a use-after-free if the can-j1939
module is unloaded while ecu->ac_timer is still armed?

When an Address Claim frame is processed in j1939_ac_process(),
j1939_ecu_timer_start() arms ecu->ac_timer for 250 ms while holding a
reference on ecu. When the last J1939 socket is closed via
j1939_sk_release() -> j1939_netdev_stop() -> __j1939_rx_release(),
ecu->ac_timer remains active and holds its reference on ecu, so
__j1939_ecu_release() is not yet called and ecu->release_work is not yet
queued on j1939_ecu_wq:

net/can/j1939/bus.c:__j1939_ecu_release() {
	...
	INIT_WORK(&ecu->release_work, __j1939_ecu_release_work);
	queue_work(j1939_ecu_wq, &ecu->release_work);
}

Neither struct j1939_ecu nor struct j1939_priv holds a module reference on
can-j1939; only open sockets hold a reference on j1939_proto.owner. If the
module is unloaded during the 250 ms window before ecu->ac_timer expires,
j1939_module_exit() destroys j1939_ecu_wq immediately because no work is
queued on it yet:

net/can/j1939/main.c:j1939_module_exit() {
	can_proto_unregister(&j1939_can_proto);

	unregister_netdevice_notifier(&j1939_netdev_notifier);

	destroy_workqueue(j1939_ecu_wq);
}

When ecu->ac_timer fires afterward, wouldn't j1939_ecu_timer_handler()
execute unmapped module text or queue ecu->release_work onto the destroyed
j1939_ecu_wq?

[ ... ]

> @@ -179,6 +188,13 @@ struct j1939_ecu *j1939_ecu_create_locked(struct j1939_priv *priv, name_t name)
>  
>  	lockdep_assert_held(&priv->lock);
>  
> +	/* Don't create a new ECU if netdev is no longer registered or
> +	 * __j1939_rx_release() was called.
> +	 */
> +	if (READ_ONCE(priv->ndev->reg_state) != NETREG_REGISTERED ||
> +	    !kref_read(&priv->rx_kref))
> +		return ERR_PTR(-ENODEV);

[Severity: High]
Does placing this check in j1939_ecu_create_locked() rather than in
j1939_local_ecu_get() still allow j1939_sk_bind() to succeed on an
unregistering net_device and leak references on struct j1939_priv, struct
j1939_ecu, and struct net_device?

In j1939_sk_bind(), the initial check of ndev->reg_state runs without
rtnl_lock before jsk is added to priv->j1939_socks (and is skipped in the
re-bind branch after j1939_jsk_del() removes jsk from priv->j1939_socks). If
unregister_netdevice() runs j1939_netdev_notify(NETDEV_UNREGISTER) ->
j1939_sk_netdev_event_unregister() before j1939_sk_bind() reaches
j1939_jsk_add(), j1939_sk_netdev_event_unregister() will not see jsk in
priv->j1939_socks.

When j1939_sk_bind() then calls j1939_local_ecu_get(),
j1939_ecu_create_locked() is bypassed whenever name == 0 or when
j1939_ecu_get_by_name_locked() finds an existing ECU in priv->ecus (such as
an ECU kept alive for 250 ms by an uncancelled ecu->ac_timer):

net/can/j1939/bus.c:j1939_local_ecu_get() {
	...
	if (!name)
		goto done;

	ecu = j1939_ecu_get_by_name_locked(priv, name);
	if (!ecu)
		ecu = j1939_ecu_create_locked(priv, name);
	...
}

net/can/j1939/socket.c:j1939_sk_bind() {
	...
	/* get new references */
	ret = j1939_local_ecu_get(priv, jsk->addr.src_name, jsk->addr.sa);
	if (ret) {
		j1939_netdev_stop(priv);
		jsk->priv = NULL;
		synchronize_rcu();
		j1939_priv_put(priv);
		goto out_release_sock;
	}

	j1939_jsk_add(priv, jsk);
	...
}

In those cases, j1939_local_ecu_get() returns 0 and j1939_jsk_add() adds jsk
after j1939_sk_netdev_event_unregister() has already finished. Should the
reg_state and rx_kref check be placed in j1939_local_ecu_get() under
priv->lock instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/3d7e48fa-9af5-43e2-84f7-884e61f9d179@I-love.SAKURA.ne.jp?part=2