Re: [PATCH net v6 1/1] can: j1939: validate the EDPO packet number

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: j1939: race on rxtimer and state in j1939_session_cancel()
--

commit c1bc44e269c86cae99ab16d5fb0f92e924343f7e
Author: Quchaosheng <quchaosheng000406@163.com>

can: j1939: validate the EDPO packet number

This commit validates the peer-controlled packet offset in ETP.CM_DPO frames
against session->pkt.total and adds a NULL skb check in
j1939_session_completed(). It also updates j1939_session_cancel() to cancel
rxtimer before taking active_session_list_lock and to deactivate sessions
already in J1939_SESSION_WAITING_ABORT.

Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..2cc6c46c4d50b 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1161,18 +1161,39 @@ static void __j1939_session_cancel(struct j1939_session *session,
>  static void j1939_session_cancel(struct j1939_session *session,
>  				 enum j1939_xtp_abort err)
>  {
> +	bool waiting;
> +
> +	/* Cancel the rxtimer before taking the list lock: hrtimer_cancel()
> +	 * waits for a running callback, and j1939_tp_rxtimer() takes the
> +	 * same lock.
> +	 */
> +	j1939_session_rxtimer_cancel(session);
>  	j1939_session_list_lock(session->priv);
>  
> +	waiting = session->state == J1939_SESSION_WAITING_ABORT;
>  	if (session->state >= J1939_SESSION_ACTIVE &&
>  	    session->state < J1939_SESSION_WAITING_ABORT) {
> -		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
>  		__j1939_session_cancel(session, err);
> +		/* hrtimer_start() does not wait for the callback, so arming
> +		 * the abort timer here cannot deadlock.
> +		 */
> +		j1939_session_get(session);
> +		hrtimer_start(&session->rxtimer,
> +			      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> +			      HRTIMER_MODE_REL_SOFT);
>  	}
>  
>  	j1939_session_list_unlock(session->priv);
>  
>  	if (!session->sk)
>  		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> +
> +	/* The abort went out on an earlier call, and this caller has just
> +	 * cancelled the timer that would have finished the session off, so
> +	 * nothing else is left to deactivate it.
> +	 */
> +	if (waiting)
> +		j1939_session_deactivate_activate_next(session);
>  }

[Severity: High]
In j1939_session_cancel(), j1939_session_rxtimer_cancel(session) is called
before acquiring active_session_list_lock, while session->rxtimer is re-armed
and waiting is evaluated under that lock.

Can this window between cancelling session->rxtimer and acquiring
active_session_list_lock introduce two races with concurrent receive packet
processing or a concurrent j1939_session_cancel() call?

First, receive handlers such as j1939_xtp_rx_dat_one(),
j1939_xtp_rx_cts_one(), and j1939_xtp_rx_dpo_one() call
j1939_tp_set_rxtimeout(session, ...) without holding active_session_list_lock.
If another processor calls j1939_tp_set_rxtimeout() after
j1939_session_rxtimer_cancel() returns here, but before
active_session_list_lock is acquired:

  j1939_session_cancel()
    j1939_session_rxtimer_cancel(session);
                                         j1939_tp_set_rxtimeout()
                                           j1939_session_rxtimer_cancel(session);
                                           j1939_session_get(session);
                                           hrtimer_start(&session->rxtimer, ...);
    j1939_session_list_lock(session->priv);
    __j1939_session_cancel(session, err);
    j1939_session_get(session);
    hrtimer_start(&session->rxtimer, ...);

Doesn't hrtimer_start() in j1939_session_cancel() re-enqueue session->rxtimer
while it is already armed, overwriting it without dropping the reference
taken by j1939_tp_set_rxtimeout() and permanently leaking session->kref
(which also holds a netdev reference on priv->ndev and blocks
unregister_netdevice())?

Second, if two callers concurrently enter j1939_session_cancel() on the same
session (for example, j1939_tp_txtimer() racing with an incoming packet),
both can execute j1939_session_rxtimer_cancel(session) before either acquires
active_session_list_lock:

  j1939_session_cancel()                 j1939_session_cancel()
    j1939_session_rxtimer_cancel()         j1939_session_rxtimer_cancel()
    j1939_session_list_lock()
    __j1939_session_cancel()
      session->state = J1939_SESSION_WAITING_ABORT;
    j1939_session_get(session);
    hrtimer_start(&session->rxtimer, ...);
    j1939_session_list_unlock()
                                           j1939_session_list_lock()
                                           waiting = session->state ==
                                             J1939_SESSION_WAITING_ABORT;
                                           j1939_session_list_unlock()
                                           if (waiting)
                                             j1939_session_deactivate_activate_next()

Wouldn't the second caller observe waiting as true and call
j1939_session_deactivate_activate_next(session) even though its
j1939_session_rxtimer_cancel() ran before the first caller armed the abort
timer, removing the session from priv->active_session_list while
session->rxtimer remains armed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179265494813.314254.14725785769836234084@163.com?part=1