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

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

[Quchaosheng]

ETP.CM_DPO is peer controlled and its packet number is accepted without
validation.  It names the packet a transmission window starts at, and
j1939_xtp_rx_dat_one() places incoming data at "dat[0] - 1 + pkt.dpo", so a
value past the end of the message moves pkt.dpo beyond the reassembled
buffer.

The transfer can still complete in that case.  With pkt.dpo == pkt.total,
a closing ETP.DT frame with dat[0] == 0 evaluates to packet pkt.total - 1,
the last in-order packet, so pkt.rx reaches pkt.total and the receiver
emits its own EOMA.  The local loopback of that EOMA runs
j1939_session_completed(), which asks j1939_session_skb_get() for offset
"pkt.dpo * 7".  That offset is past the end of the queued skb, the lookup
returns NULL, and the NULL reached j1939_sk_recv(), which dereferenced
oskb->sk.

  ETP.CM_RTS (size 1786)  -> total = 256 packets
  ETP.CM_DPO (packet 0)
  ETP.DT seq 1..255       -> packets 0..254, pkt.rx = 255
  ETP.CM_DPO (packet 256) -> pkt.dpo = 256, unvalidated
  ETP.DT seq 0            -> packet 255 == pkt.rx, completes the transfer

Reject an EDPO that names a packet at or past pkt.total.  The check is ">="
and not ">": pkt.total is a packet count, so the last valid packet is
pkt.total - 1.

The abort code depends on the role.  A receiver is told
J1939_XTP_ABORT_BAD_EDPO_OFFSET.  A transmitter only sees an out-of-range
EDPO on the echo of one it built itself, from an ETP.CM_CTS packet number
that j1939_xtp_rx_cts_one() does not bound, so the responder is told
J1939_XTP_ABORT_ECTS_TOO_BIG, the code j1939_tp_txtimer() already uses for
that condition.  Bounding ETP.CM_CTS is a separate change; the comment in
j1939_xtp_rx_dpo_one() says so, because it is the other writer of pkt.dpo.

j1939_session_cancel() changes in two ways:

  - It cancels the rxtimer before taking active_session_list_lock.
    j1939_tp_set_rxtimeout() cancels synchronously, hrtimer_cancel() waits
    for a running callback, and j1939_tp_rxtimer() takes that same lock.
    j1939_tp_txtimer()'s -EOVERFLOW and default branches reach
    j1939_session_cancel() without cancelling the timer first.  The abort
    timer is then armed with hrtimer_start() under the lock, which does not
    wait for the callback.

  - It deactivates a session that is already in
    J1939_SESSION_WAITING_ABORT.
    Callers cancel the timers first, so a second cancel otherwise leaves an
    active session with nothing left to deactivate it, including on the
    ETP.DT and ETP.CM_CTS paths that do not use the new EDPO branch.  With
    that in the helper, the second-RTS path's receiver branch reduces to
    what its transmitter branch already does.

Guard j1939_session_completed() against a NULL skb as defense in depth, so
future regression in the offset arithmetic drops the message instead of
panicking.

Tested on v7.3-rc7 under QEMU (KVM, vcan).  Three kernels from one tree and
one configuration (defconfig plus CONFIG_CAN_J1939=y, CONFIG_CAN_VCAN=y,
CONFIG_LOCKDEP=y, CONFIG_DEBUG_SPINLOCK=y), differing only in this hunk:
stock, v5, and this revision.  Each case is a standalone guest.

  T1  out-of-range EDPO (DPO 256, then DT seq 0)
  T2  legal 1786-byte ETP transfer, payload compared byte for byte
  T3  abort EDPO, then an unparsable DT; is a second ETP.CM_RTS from the
      same SA/DA answered with a CTS?
  T4  same as T3, last socket closed, does `ip link del vcan0` return?
  T5  two unparsable DTs, no EDPO; second RTS answered?
  T6  two unparsable DTs; does `ip link del vcan0` return?
  T7  oversized ETP.CM_CTS against a real sender: which abort code?

            T1       T2     T3          T5          T7
  stock     panic    -      SLOT-STUCK  SLOT-STUCK  -
  v5        ok       PASS   SLOT-STUCK  SLOT-STUCK  12
  this rev  ok       PASS   SLOT-FREE   SLOT-FREE   15

            T4             T6
  stock     REF-DROPPED*   never returns
  v5        never returns  never returns
  this rev  returns        returns

* T4 on stock aborts with code 6, UNEXPECTED_DATA, which leaves the session
  to the abort timer, and the timer deactivates it as the device goes down.
  T3 is still stuck on stock, so the slot is not reusable and a peer that
  restarts the transfer is refused until the timeout.

T1 on stock is "BUG: kernel NULL pointer dereference, address: 0x18" in
j1939_sk_recv(), then a kernel panic.  T2 delivers all 256 packets intact
on both patched kernels.

T5 and T6 strand the session on stock with no EDPO involved: the second
unparsable DT cancels the abort timer the first one armed, and
j1939_session_cancel() finds the state already WAITING_ABORT.  T6 prints
"unregister_netdevice: waiting for vcan0 to become free. Usage count = 7"
until the guest is killed.  v5 fixed only the caller it added, so T3
through T6 are unchanged from stock.  This revision deactivates in the
helper, and all four clear.

The lockdep race that the first change addresses was not reproduced: it
needs a timer callback racing a caller, and none of these sequences
schedules one at the right moment.  No lockdep report or call trace
appeared in any run.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 81 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 15 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..2cc6c46c4d50 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);
 }
 
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
@@ -1243,9 +1264,17 @@ static void j1939_session_completed(struct j1939_session *session)
 
 	if (!session->transmission) {
 		se_skb = j1939_session_skb_get(session);
-		/* distribute among j1939 receivers */
-		j1939_sk_recv(session->priv, se_skb);
-		consume_skb(se_skb);
+
+		/* Defense in depth: pkt.dpo is range checked when an EDPO is
+		 * received, so the lookup should always find the skb.  Drop
+		 * the message rather than hand a NULL skb to j1939_sk_recv()
+		 * if the offset arithmetic ever regresses.
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
@@ -1729,16 +1758,10 @@ static int j1939_xtp_rx_rts_session_active(struct j1939_session *session,
 
 		j1939_session_timers_cancel(session);
 		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
-		if (session->transmission) {
-			j1939_session_deactivate_activate_next(session);
-		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
-			/* Force deactivation for the receiver.
-			 * If we rely on the timer starting in j1939_session_cancel,
-			 * a second RTS call here will cancel that timer and fail
-			 * to restart it because the state is already WAITING_ABORT.
-			 */
-			j1939_session_deactivate_activate_next(session);
-		}
+		/* Both roles: the peer is restarting the transfer, so free the
+		 * slot now rather than wait out the abort timeout.
+		 */
+		j1939_session_deactivate_activate_next(session);
 
 		return -EBUSY;
 	}
@@ -1823,14 +1846,42 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 				 struct sk_buff *skb)
 {
 	const u8 *dat = skb->data;
+	unsigned int dpo;
 
 	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
 		return;
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	dpo = j1939_etp_ctl_to_packet(dat);
+
+	/* EDPO is peer controlled.  j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so a value at or past pkt.total moves
+	 * pkt.dpo past the reassembled buffer, and the ETP completion path
+	 * then looks up an offset no queued skb covers.
+	 *
+	 * This bounds the receiver-side writer only.  j1939_session_tx_dpo()
+	 * copies pkt.tx_acked, which comes from an ETP.CM_CTS packet number
+	 * that j1939_xtp_rx_cts_one() does not bound; that is a separate fix.
+	 */
+	if (dpo >= session->pkt.total) {
+		netdev_warn(session->priv->ndev,
+			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
+			    __func__, session, dpo, session->pkt.total);
+		j1939_session_timers_cancel(session);
+		/* The transmitter only sees this on the echo of an EDPO it built
+		 * from an oversized ETP.CM_CTS packet number, so it is the
+		 * responder's CTS that is too big, not the EDPO offset.
+		 */
+		j1939_session_cancel(session,
+				     session->transmission ?
+				     J1939_XTP_ABORT_ECTS_TOO_BIG :
+				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);
+		return;
+	}
+
 	/* transmitted without problems */
-	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	session->pkt.dpo = dpo;
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 

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

[sashiko-bot]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Deactivating a WAITING_ABORT session straight away in
  j1939_session_cancel() can let that session's own ABORT echo, still in…
- [Medium] j1939_session_cancel() now cancels the rxtimer at line 1170,
  outside active_session_list_lock.
- [Low] The new comment in j1939_xtp_rx_dpo_one() says the transmitter
  'only sees this on the echo of an EDPO it built from an oversized…
- [Low] The subject 'can: j1939: validate the EDPO packet number' covers
  only one of several independent fixes in this patch.
- [Low] Commit message process problems: (1) The body includes patch-
  revision history and a test matrix comparing revisions ('stock, v5,…

--

can: j1939: validate the EDPO packet number

This commit makes j1939_xtp_rx_dpo_one() reject an ETP.CM_DPO whose packet
number is at or past pkt.total. That stops a NULL skb from reaching
j1939_sk_recv() when the transfer completes. It also changes
j1939_session_cancel() to cancel the rxtimer outside
active_session_list_lock and to deactivate sessions already in
J1939_SESSION_WAITING_ABORT, simplifies second-RTS handling, and adds a
NULL guard in j1939_session_completed().

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

> can: j1939: validate the EDPO packet number

[Severity: Low]

This isn't a bug, but the subject only describes the EDPO bound check. The
patch also carries several independent changes:

- j1939_session_cancel() now deactivates sessions already in
  J1939_SESSION_WAITING_ABORT. This fixes an existing session/netdev
  reference strand that needs no EDPO. In the T5/T6 cases, two unparsable
  ETP.DT frames leave ip link del stuck on "unregister_netdevice: waiting
  for vcan0 to become free".

- The rxtimer cancel moves out from under active_session_list_lock. This
  addresses the hrtimer_cancel() vs j1939_tp_rxtimer() wait on the
  j1939_tp_txtimer() -EOVERFLOW and default paths.

- The second-RTS handling in j1939_xtp_rx_rts_session_active() is
  simplified.

- A NULL guard is added in j1939_session_completed().

The dpo >= pkt.total check by itself is enough to stop the T1 NULL
dereference.

Could the WAITING_ABORT deactivation and the timer/lock ordering change be
split into their own patches with their own subjects? That would let them
be bisected, reviewed and backported separately.

> Tested on v7.3-rc7 under QEMU (KVM, vcan).  Three kernels from one tree and
> one configuration (defconfig plus CONFIG_CAN_J1939=y, CONFIG_CAN_VCAN=y,
> CONFIG_LOCKDEP=y, CONFIG_DEBUG_SPINLOCK=y), differing only in this hunk:
> stock, v5, and this revision.  Each case is a standalone guest.

[ ... ]

> until the guest is killed.  v5 fixed only the caller it added, so T3
> through T6 are unchanged from stock.  This revision deactivates in the
> helper, and all four clear.

[ ... ]

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3
> Assisted-by: LLM
> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]

A few commit message process points. None of them affect the code.

The body compares patch revisions ("stock, v5, and this revision", "v5
fixed only the caller it added"). Could this history move below the ---
line, as Documentation/process/submitting-patches.rst describes?

The Author: line is "Quchaosheng <quchaosheng000406@163.com>", but the
Signed-off-by: line is "Chaosheng Qu <quchaosheng000406@163.com>". The email
is the same, but checkpatch will report a From:/Signed-off-by: name
mismatch.

Reported-by: is followed by Link: and not by the Closes: tag that
checkpatch's BAD_REPORTED_BY_LINK check expects.

> 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);
>  	}

[Severity: Medium]

Can this leak a session reference? That happens if another path arms the
rxtimer between the cancel at the top of j1939_session_cancel() and this
get+start.

The cancel now runs before spin_lock_bh(). The get+start runs only after
__j1939_session_cancel() has sent the ABORT frame and called
j1939_sk_send_loop_abort().

The other arming path does not take active_session_list_lock:

j1939_tp_set_rxtimeout() {
	j1939_session_rxtimer_cancel(session);
	j1939_session_get(session);
	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);
}

It is called without that lock from j1939_xtp_rx_cts_one(),
j1939_xtp_rx_dpo_one() and j1939_xtp_rx_dat_one(). So holding the list lock
here does not serialize against it. For example:

CPU0: j1939_tp_txtimer(), j1939_session_tx_dat() returned -EOVERFLOW
  j1939_session_cancel()
    j1939_session_rxtimer_cancel()  /* hrtimer_cancel() returns 0 */
    spin_lock_bh(), __j1939_session_cancel() sends the ABORT

CPU1: ETP.CM_CTS for the same session
  j1939_xtp_rx_cts_one()
    j1939_tp_set_rxtimeout()
      j1939_session_get()           /* +1 */
      hrtimer_start()

CPU0:
    j1939_session_get()             /* +1 */
    hrtimer_start()                 /* re-queues the queued timer */

One expiry of j1939_tp_rxtimer() drops only one reference. The session holds
a netdev reference through priv_dev_tracker and also a priv reference. Would
a later ip link del then hang in unregister_netdevice?

The old code had the same cancel/get/start pattern. There it ran back to
back inside j1939_tp_set_rxtimeout(). This patch widens the window to cover
lock contention and a CAN frame transmit.

The commit message says "The abort timer is then armed with hrtimer_start()
under the lock", but the lock does not serialize rxtimer arming. One option
is to call hrtimer_try_to_cancel() under the lock, since it does not wait.
The reference would be put if it returns 1, before the get+start.

>  
>  	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: Medium]

Could deactivating the WAITING_ABORT session here let its own ABORT echo,
still in flight, cancel the next transfer queued on the same socket?

Consider a socket with transmitter session A active and session B queued
behind it. Both use the same SA/DA, the same TP/ETP type and the same PGN,
which j1939_sk_queue_session() allows. The peer sends two malformed
ETP.CM_CTS frames back to back, for example with packet number 0.

First CTS:

j1939_xtp_rx_cts_one()
  goto out_session_cancel
  j1939_session_cancel()
    __j1939_session_cancel()
      j1939_xtp_tx_abort()       /* ABORT sent, echo is async */
    A->state = J1939_SESSION_WAITING_ABORT

The second CTS arrives before the echo is processed.
j1939_session_get_by_addr() still finds A, because WAITING_ABORT sessions
stay on active_session_list. last_cmd was not updated, so:

j1939_xtp_rx_cts_one()
  goto out_session_cancel
  j1939_session_timers_cancel()  /* kills the abort timer */
  j1939_session_cancel()
    if (waiting)
      j1939_session_deactivate_activate_next(A)
        j1939_sk_queue_activate_next_locked()
          j1939_session_activate(B)  /* B is ACTIVE on the list */

Only B's txtimer is delayed, by 10-25 ms. A's ABORT echo then arrives with
J1939_ECU_LOCAL_SRC set:

j1939_tp_cmd_recv()
  j1939_xtp_rx_abort(priv, skb, true)
    j1939_xtp_rx_abort_one()
      j1939_session_get_by_addr(..., false, true)  /* matches B */
      j1939_xtp_rx_cmd_bad_pgn()                   /* PGNs equal */

B is then aborted, and its sender gets an error before B has sent its RTS.
Before this patch, the second CTS left A on the list, so the echo matched A
and cleaned it up.

[ ... ]

> @@ -1823,14 +1846,42 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

> +	if (dpo >= session->pkt.total) {
> +		netdev_warn(session->priv->ndev,
> +			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
> +			    __func__, session, dpo, session->pkt.total);
> +		j1939_session_timers_cancel(session);
> +		/* The transmitter only sees this on the echo of an EDPO it built
> +		 * from an oversized ETP.CM_CTS packet number, so it is the
> +		 * responder's CTS that is too big, not the EDPO offset.
> +		 */
> +		j1939_session_cancel(session,
> +				     session->transmission ?
> +				     J1939_XTP_ABORT_ECTS_TOO_BIG :
> +				     J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: Low]

Is the claim in this comment accurate? j1939_can_recv() sets
J1939_ECU_LOCAL_SRC based on the SA alone:

	if (j1939_address_is_unicast(skcb->addr.sa) &&
	    priv->ents[skcb->addr.sa].nusers)
		skcb->flags |= J1939_ECU_LOCAL_SRC;

j1939_tp_cmd_recv() then sends any ETP.CM_DPO with that flag to the
transmitter session:

		if (j1939_tp_im_transmitter(skcb))
			j1939_xtp_rx_dpo(priv, skb, true);

This means any node on the bus that sends an out-of-range DPO with the local
SA reaches this branch. It is not limited to the echo of a locally built
EDPO. In that case the transmitter sends J1939_XTP_ABORT_ECTS_TOO_BIG to
the responder for a CTS the responder never sent.

The abort itself still happens. Only the comment and the abort reason code
look wrong. Could the comment, and the matching sentence in the commit
message, be reworded? Or could the code be chosen differently?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179265494813.314254.14725785769836234084%40163.com

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

[Chaosheng Qu]

Hi sashiko,

Thanks -- both Medium findings are real. I will respin as a split series
rather than argue with them, and the three Low findings are accepted too; two
of them are process points that I should simply have gotten right the first
time.

## Medium: session reference leak in `j1939_session_cancel()`

Accepted. The old code did the cancel/get/start back to back inside
`j1939_tp_set_rxtimeout()`. The patch moves the cancel above
`j1939_session_list_lock()` and the get+start below `__j1939_session_cancel()`,
which calls `j1939_xtp_tx_abort()` -> `j1939_xtp_do_tx_ctl()` ->
`can_send()`, and then `j1939_sk_send_loop_abort()`. So the window now spans
lock acquisition and a CAN frame transmit instead of three instructions.

The list lock does not help, exactly as you say: `j1939_tp_set_rxtimeout()`
does not take it, and it is reached from the receive path without it by
`j1939_xtp_rx_cts_one()`, `j1939_xtp_rx_dpo_one()` and
`j1939_xtp_rx_dat_one()`, plus from the transmit retry path through
`j1939_session_tx_rts()`/`_tx_dpo()`/`_tx_dat()` inside `j1939_tp_txtimer()`.
Your CPU0/CPU1 interleaving is therefore reachable: two `j1939_session_get()`
calls, one queued hrtimer, one `j1939_session_put()` on expiry. The session
then never reaches `j1939_session_destroy()`, so it keeps both its
`priv_dev_tracker` netdev reference and its `j1939_priv` reference, and
`ip link del` hangs in `unregister_netdevice()`.

Two notes on the fix I intend to take, because I do not think
`hrtimer_try_to_cancel()` alone closes it:

- The reason the cancel had to move out of the lock stays valid. Before this
  patch, `j1939_session_cancel()` called `hrtimer_cancel(&session->rxtimer)`
  while holding `active_session_list_lock`, and `j1939_tp_rxtimer()` takes the
  same lock, so a running softirq callback waiting for the lock deadlocks the
  CPU that holds the lock and waits for the callback.
- `hrtimer_try_to_cancel()` under the lock avoids the deadlock -- a callback
  that is already running makes it return -1 instead of waiting -- but the get
  and the start would still race with a concurrent `j1939_tp_set_rxtimeout()`,
  which does its own cancel/get/start without the lock. Closing the window
  needs both armers serialized, so the respin arms the rxtimer only under
  `active_session_list_lock`: `j1939_tp_set_rxtimeout()` takes the lock and
  uses `hrtimer_try_to_cancel()` + `j1939_session_put()` on a return of 1
  before the get+start, and `j1939_session_cancel()` does the same under the
  lock it already takes. That is the pattern `j1939_cancel_active_session()`
  already uses. No caller of `j1939_tp_set_rxtimeout()` holds that lock today,
  so this does not create a new lock order.

That is a larger change than the current hunk, which is another reason to
split it out (see the subject finding below).

## Medium: WAITING_ABORT deactivation lets the ABORT echo hit the next session

Accepted, and this one is the more serious of the two. Confirmed in the tree:
`j1939_xtp_rx_abort()` calls `j1939_xtp_rx_abort_one()` twice, once with
`reverse = false` and once with `true`, and each does
`j1939_session_get_by_addr()`, which walks `active_session_list` and matches on
SA/DA/type/PGN only. Queued sessions are activated by
`j1939_sk_queue_activate_next_locked()`, which schedules the next session's
txtimer with `time_ms = 10 + get_random_u32_below(16)` when the previous
session carried an error, so there is a real 10-25 ms window in which B is on
the list and has not sent anything yet. An ABORT echo that is still in flight
from A then matches B, `j1939_xtp_rx_abort_one()` drops B with
`j1939_session_timers_cancel()` + `j1939_session_deactivate_activate_next()`,
and the sender of B gets an error for a transfer that never started.

The respin drops the `waiting` branch entirely and instead arms the abort
timer for both `J1939_SESSION_ACTIVE` and `J1939_SESSION_WAITING_ABORT`. Only
the `ACTIVE` case calls `__j1939_session_cancel()`, so a second error input
does not put a second ABORT frame on the bus; it just re-arms the timer. The
session stays on `active_session_list` either way. What the branch was working
around is that a second error input reaching the same session calls
`j1939_session_timers_cancel()` first, which cancels the abort timer armed by
the first call, and the second `j1939_session_cancel()` then found the state
already `J1939_SESSION_WAITING_ABORT`, matched neither arm of the old state
test, and armed nothing -- the stranding case from your subject finding, the
one that ends in `unregister_netdevice: waiting for vcan0 to become free`. With
the timer re-armed, the session either gets its own ABORT echo, which now still
finds it on the list because it was never deactivated, or `j1939_tp_rxtimer()`
takes the `J1939_SESSION_WAITING_ABORT` branch after
`J1939_XTP_ABORT_TIMEOUT_MS` and deactivates as before. Each re-arm stays
balanced: one `j1939_session_get()` against the one `j1939_session_put()` in
the callback.

## Low: the comment in `j1939_xtp_rx_dpo_one()` is wrong

Accepted, the comment is simply incorrect. `j1939_can_recv()` sets
`J1939_ECU_LOCAL_SRC` from the source address alone:

```c
	if (j1939_address_is_unicast(skcb->addr.sa) &&
	    priv->ents[skcb->addr.sa].nusers)
		skcb->flags |= J1939_ECU_LOCAL_SRC;
```

and `j1939_tp_cmd_recv()` forwards every ETP.CM_DPO with that flag to the
transmitter session, so any local node sending a bad DPO reaches the branch,
not only the echo of an EDPO this stack built. Both the comment and the
matching sentence in the commit message go away.

On the reason code: `J1939_XTP_ABORT_ECTS_TOO_BIG` ("ECTS requested packets
exceeds message size") is wrong for a DPO we received, so the check will send
`J1939_XTP_ABORT_BAD_EDPO_OFFSET` unconditionally. The transmitter-side case
that motivated the conditional is real, but its cause is not in this function:
`j1939_xtp_rx_cts_one()` bounds `pkt` only with `if (!pkt)` and
`dat[1] > session->pkt.block`, so an oversized ETP.CM_CTS packet number can set
`pkt.tx_acked` past `pkt.total`, and the EDPO built from it in
`j1939_session_tx_dpo()` carries that value back to us. The respin bounds the
CTS there -- `pkt > session->pkt.total` is exactly `ECTS_TOO_BIG` -- and then an
out-of-range DPO reaching the transmitter is a genuine peer error.

## Low: the subject covers only one of the changes

Accepted. v7 will be four patches:

1. `can: j1939: abort a session whose abort timer was cancelled` -- the
   WAITING_ABORT re-arm, with the two-unparsable-ETP.DT repro and the
   `ip link del` hang.
2. `can: j1939: arm the rx timer under the session list lock` -- the
   `j1939_tp_set_rxtimeout()` / `j1939_session_cancel()` serialization above.
3. `can: j1939: simplify the second RTS handling` -- the
   `j1939_xtp_rx_rts_session_active()` change.
4. `can: j1939: validate the EDPO packet number` -- the bound check, the NULL
   guard in `j1939_session_completed()`, and the CTS bound; the 256-packet
   repro and the 3/3 panic result stay with this one.

You are right that the `dpo >= pkt.total` check alone stops the T1 NULL
dereference; patches 1 and 2 are there for the reference strand, not for T1,
and I will say so in their messages instead of implying they are part of the
crash fix.

## Low: commit message and tags

Accepted, all three:

- The revision and testing history ("stock, v5, and this revision", the
  per-case matrix) moves below the `---` line.
- `From:` and `Signed-off-by:` will use the same name. The `Author:
  Quchaosheng` in v6 is a `git send-email` identity mismatch on my side, not
  an intentional alias.
- `Closes:` replaces `Link:` after `Reported-by:`.

Thanks for the review -- the two Medium findings in particular were worth the
round trip, and finding 3 saved a wrong abort code from going in.

Chaosheng

pw-bot: cr