Re: [PATCH] can: j1939: fix memory leaks caused by pending address claim timer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Ignoring hrtimer_try_to_cancel() failure introduces race conditions leading to inappropriate ECU mapping (NETDEV_DOWN, IDLE claims, and delay bypass).

Pre-existing issues:
- [Critical] Lockless list_del in __j1939_ecu_release leads to linked list corruption and use-after-free.
--

commit 2b6762bc65c5c7e152f9be9bd0a6d303989290db
Author: Slawomir Stepien <sst@poczta.fm>

can: j1939: fix memory leaks caused by pending address claim timer

Fixes a memory leak and deadlock involving pending Address Claim timers during
netdev teardown. It cancels timers during j1939_ecu_unmap_all() and adds
kref_read(&priv->rx_kref) checks to prevent remapping ECUs on stopped
interfaces.

Link: https://syzkaller.appspot.com/ai_job?id=c118979e-3d39-430b-b07e-608d26aeaa70

> diff --git a/net/can/j1939/bus.c b/net/can/j1939/bus.c
> index cdc3c0a719379..71e52f25aa604 100644
> --- a/net/can/j1939/bus.c
> +++ b/net/can/j1939/bus.c
[ ... ]
> @@ -121,7 +126,7 @@ void j1939_ecu_timer_start(struct j1939_ecu *ecu)
>  
>  void j1939_ecu_timer_cancel(struct j1939_ecu *ecu)
>  {
> -	if (hrtimer_cancel(&ecu->ac_timer))
> +	if (hrtimer_try_to_cancel(&ecu->ac_timer) == 1)
>  		j1939_ecu_put(ecu);
>  }

[Severity: High]
Does ignoring the -1 return value from hrtimer_try_to_cancel() introduce race
conditions that could lead to inappropriate ECU mapping?

The patch replaces a deadlocking hrtimer_cancel() with hrtimer_try_to_cancel()
but ignores failures. If the timer callback is running concurrently on another
CPU, it will block on write_lock_bh(&priv->lock). 

If the current CPU executes j1939_ecu_unmap_all() or processes an IDLE claim
in j1939_ac_process(), it holds priv->lock, calls hrtimer_try_to_cancel()
(which fails and returns -1), modifies the state, and unlocks.

The other CPU running the timer callback then acquires the lock and could
execute j1939_ecu_map_locked() based on old state. Could this cause the ECU to
be mapped back onto a DOWN interface, ignore IDLE claims by re-mapping the old
address, or bypass the J1939 250ms delay by immediately mapping the new
address?

[ ... ]
> @@ -131,17 +136,18 @@ static enum hrtimer_restart j1939_ecu_timer_handler(struct hrtimer *hrtimer)
>  		container_of(hrtimer, struct j1939_ecu, ac_timer);
>  	struct j1939_priv *priv = ecu->priv;
>  
> +	j1939_priv_get(priv);
>  	write_lock_bh(&priv->lock);
> -	/* TODO: can we test if ecu->addr is unicast before starting
> -	 * the timer?
> -	 */
> -	j1939_ecu_map_locked(ecu);
> +	/* Only map the ECU if the netdev is still active */
> +	if (kref_read(&priv->rx_kref) > 0)
> +		j1939_ecu_map_locked(ecu);
>  
>  	/* The corresponding j1939_ecu_get() is in
>  	 * j1939_ecu_timer_start().
>  	 */
>  	j1939_ecu_put(ecu);

[Severity: Critical]
This is a pre-existing issue, but could dropping the last ECU reference
outside of priv->lock lead to list corruption?

In __j1939_ecu_release(), there is a lockless call to list_del(&ecu->list).
If CPU 1 receives an address claim, j1939_ac_recv() takes a temporary
reference via j1939_ecu_get_by_addr(). If CPU 2 concurrently executes
j1939_ecu_unmap_all(), unmapping the ECU and dropping its mapping reference,
then when CPU 1 finishes processing it calls j1939_ecu_put(), dropping the
final reference.

Because paths like j1939_ac_recv() and j1939_ac_fixup() call
j1939_ecu_put(ecu) without holding priv->lock, does this cause
__j1939_ecu_release() to execute list_del(&ecu->list) locklessly, resulting
in list corruption and use-after-free crashes on subsequent traversals?

>  	write_unlock_bh(&priv->lock);
> +	j1939_priv_put(priv);
>  
>  	return HRTIMER_NORESTART;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1ca51efc-8929-4df1-ab11-72407b6d496f@mail.kernel.org?part=1