Re: [PATCH net v5 2/3] can: j1939: cancel TX timer after RX timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] can: j1939: race in j1939_session_txtimer_cancel_async_locked() drops kref
--

commit 6ad432b9ff1c3b364014147532b4d663b71e57c7
Author: Zihan Xi <zihanx@nebusec.ai>

can: j1939: cancel TX timer after RX timeout

This commit cancels a J1939 session's TX timer asynchronously via
j1939_session_txtimer_cancel_async() when an RX timeout moves an active
session to J1939_SESSION_WAITING_ABORT in j1939_tp_rxtimer().

Link: https://bugtracker.nebusec.ai/f/11143

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 4958f7c47ca44..c85d7bb3c12d4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1387,6 +1387,7 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  						     struct j1939_session,
>  						     rxtimer);
>  	struct j1939_priv *priv = session->priv;
> +	bool cancelled = false;
>  
>  	if (session->state == J1939_SESSION_WAITING_ABORT) {
>  		netdev_alert(priv->ndev, "%s: 0x%p: abort rx timeout. Force session deactivation\n",
> @@ -1414,8 +1415,11 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
>  				      HRTIMER_MODE_REL_SOFT);
>  			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
> +			cancelled = true;
>  		}
>  		j1939_session_list_unlock(session->priv);
> +		if (cancelled)
> +			j1939_session_txtimer_cancel_async(session);

[Severity: Critical]
Can calling j1939_session_txtimer_cancel_async() here race with a concurrent
j1939_session_txtimer_cancel() and re-arm session->txtimer without holding a
session kref, leading to a use-after-free of struct j1939_session?

Specifically, j1939_session_txtimer_cancel_async() calls
j1939_session_txtimer_cancel_async_locked():

net/can/j1939/transport.c:j1939_session_txtimer_cancel_async_locked() {
	lockdep_assert_held(&session->txtimer_lock);
	session->txtimer_shutdown = true;
	ret = hrtimer_try_to_cancel(&session->txtimer);
	/* Let a queued successor finish promptly after callback exit. */
	if (ret < 0 && session->txtimer_ref &&
	    hrtimer_is_queued(&session->txtimer))
		hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT);
	...
}

While j1939_session_txtimer_cancel() waits for a running callback outside
session->txtimer_lock:

net/can/j1939/transport.c:j1939_session_txtimer_cancel() {
	...
	/* Never wait for a softirq callback while holding txtimer_lock. */
	if (ret < 0) {
		ret = hrtimer_cancel(&session->txtimer);
		if (ret == 1) {
			spin_lock_bh(&session->txtimer_lock);
			if (!WARN_ON_ONCE(!session->txtimer_ref)) {
				session->txtimer_ref = false;
				put_ref = true;
			}
			spin_unlock_bh(&session->txtimer_lock);
		}
	}
	...
}

Consider the following sequence across three CPUs when an RX timeout occurs
while j1939_tp_txtimer() is completing and a synchronous cancellation (for
example via j1939_xtp_rx_abort_one() on the abort frame sent by
__j1939_session_cancel(), or via an incoming CTS/RTS frame) runs
concurrently:

1. On CPU 0, j1939_tp_txtimer() is running, has queued a successor via
   j1939_tp_schedule_txtimer() (so session->txtimer is queued and
   session->txtimer_ref is true), releases session->txtimer_lock, and is
   about to finish __run_hrtimer().

2. On CPU 1, j1939_session_txtimer_cancel() increments
   session->txtimer_cancelers, sees hrtimer_try_to_cancel() return -1 because
   CPU 0 has not cleared base->running yet, drops session->txtimer_lock, and
   calls hrtimer_cancel(&session->txtimer).

3. On CPU 2, j1939_tp_rxtimer() calls j1939_session_txtimer_cancel_async(),
   which acquires session->txtimer_lock and enters
   j1939_session_txtimer_cancel_async_locked(). It also sees
   hrtimer_try_to_cancel() return -1 while CPU 0 is still in __run_hrtimer(),
   and evaluates ret < 0 && session->txtimer_ref &&
   hrtimer_is_queued(&session->txtimer) as true without checking
   session->txtimer_cancelers.

4. CPU 0 finishes __run_hrtimer() and clears base->running. On CPU 1,
   hrtimer_cancel(&session->txtimer) immediately succeeds in cancelling the
   queued timer and returns 1.

5. CPU 2 calls hrtimer_start(&session->txtimer, 0, HRTIMER_MODE_REL_SOFT),
   re-enqueuing session->txtimer, and releases session->txtimer_lock.

6. CPU 1 acquires session->txtimer_lock, sees ret == 1 and
   session->txtimer_ref == true, sets session->txtimer_ref = false, and drops
   the timer's session reference via j1939_session_put(session).

At that point, session->txtimer is queued in the hrtimer subsystem with
session->txtimer_ref == false and no kref pinning the session. Should
j1939_session_txtimer_cancel_async_locked() avoid calling hrtimer_start()
when session->txtimer_cancelers is non-zero (or otherwise synchronize with
concurrent cancelers)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793681851.git.zihanx@nebusec.ai?part=2