Re: [PATCH v4] can: j1939: cancel all pending ECUs on device stop

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] can: j1939: refcount underflow of ECU on inactive timer cancel
- [High] can: j1939: unintended global disruption of active address claims
- [Medium] can: j1939: memory leak of ECU from ignored running timer
--

commit 8541ad61eedf789fa9a61f3ebd58354bc6c07318
Author: Dmitry Antipov <dmantipov@yandex.ru>

can: j1939: cancel all pending ECUs on device stop

Cancel all pending ECUs in j1939_netdev_stop() to prevent memory leaks when a
stopped device no longer processes ECUs. This introduces j1939_ecu_cancel_all()
using hrtimer_try_to_cancel() to avoid deadlocks.

Reported-by: syzbot+489e907b2a026a6f5fa0@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=489e907b2a026a6f5fa0
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..0b5232b536051 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
> @@ -96,6 +96,17 @@ void j1939_ecu_unmap(struct j1939_ecu *ecu)
>  	write_unlock_bh(&ecu->priv->lock);
>  }
>  
> +void j1939_ecu_cancel_all(struct j1939_priv *priv)
> +{
> +	struct j1939_ecu *ecu, *tmp;
> +
> +	write_lock_bh(&priv->lock);
> +	list_for_each_entry_safe(ecu, tmp, &priv->ecus, list)
> +		if (hrtimer_try_to_cancel(&ecu->ac_timer) != -1)
> +			j1939_ecu_put(ecu);

[Severity: Critical]
Does this code inadvertently drop a reference that the timer does not own?

If the timer is inactive, hrtimer_try_to_cancel() returns 0. Since 0 is != -1,
the condition evaluates to true, and j1939_ecu_put() is called unconditionally
for inactive timers. Could this lead to a refcount underflow and use-after-free
if a user binds and then closes a socket?

[Severity: Medium]
Will this leak the ECU and the associated j1939_priv if the timer callback is
currently running?

If hrtimer_try_to_cancel() returns -1, the -1 state is dropped without waiting
or retrying, abandoning the active timer during teardown.

If the abandoned callback j1939_ecu_timer_handler() begins executing and later
calls j1939_ecu_map_locked() after unmap_all has finished, might this map the
ECU back into the stopped device and prevent it from ever being freed?

> +	write_unlock_bh(&priv->lock);
> +}
> +

[ ... ]

> diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
> index 5e5e6c228f225..1f68ba8037004 100644
> --- a/net/can/j1939/main.c
> +++ b/net/can/j1939/main.c
> @@ -306,6 +306,7 @@ struct j1939_priv *j1939_netdev_start(struct net_device *ndev)
>  
>  void j1939_netdev_stop(struct j1939_priv *priv)
>  {
> +	j1939_ecu_cancel_all(priv);
>  	kref_put_mutex(&priv->rx_kref, __j1939_rx_release, &j1939_netdev_lock);

[Severity: High]
Can this unintentionally cancel all ECU timers interface-wide on a single
socket's close event before checking the interface reference count?

Because j1939_ecu_cancel_all(priv) is called unconditionally before the
priv->rx_kref reference check, any user creating and closing a socket could
unintentionally disrupt network interface state and abort in-progress address
claims for all other applications using the same interface.

>  	j1939_priv_put(priv);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002091552.737529-1-dmantipov@yandex.ru?part=1