[PATCH net v7 0/4] can: j1939: fix the abort timer and bound EDPO

[PATCH net v7 0/4] can: j1939: fix the abort timer and bound EDPO

[Chaosheng Qu]

v7 of the j1939 EDPO fix.  Two findings from the automated review of v6 were
real and both were in the session cancel path, so this is now a series:
patch 1 is the timer fix, patch 2 is the original EDPO bound check.

  [1/4] arms the abort timer under the session list lock.  Fixes two things
        at once: the hrtimer_cancel()/j1939_tp_rxtimer() deadlock, and the
        reference leak when a second unparsable frame re-enters the cancel
        path after the timers were killed.

  [2/4] is the original change: reject an ETP.CM_DPO that names a packet at
        or past pkt.total.  Without it a peer can move pkt.dpo past the end
        of the reassembled buffer, and the local EOMA loopback then hands a
        NULL skb to j1939_sk_recv().

  [3/4] guards j1939_session_completed() against that NULL anyway.  With
        [2/4] in place this should be unreachable; it turns a future
        regression in the offset arithmetic into a dropped datagram instead
        of a kernel panic.

  [4/4] rejects an oversized ETP.CM_CTS at the point where it is parsed,
        instead of letting it travel back as a bad EDPO offset.

Testing:

  - [2/4] alone was verified against a local vcan setup: an ETP session with
    ETP.CM_RTS size 1786 (256 packets) followed by ETP.CM_DPO packet 256 and
    an ETP.DT with sequence 0 gave a NULL dereference in j1939_sk_recv()
    3/3 times before the change and was rejected 3/3 times after it.

  - The T5/T6 hang from the review ([1/4]): two unparsable ETP.DT frames
    followed by "ip link del" left unregister_netdevice() waiting for the
    session reference; with the rearmed timer the session is deactivated
    500 ms after the second frame.

  - Each patch builds standalone; net/can/j1939/ is unchanged in behaviour
    for anything that does not involve an EDPO, a CTS or an abort.

Changes in v7:

  - Split into four patches, as promised in the reply to the v6 review.
  - [1/4] replaces the "waiting" branch with rearming the abort timer under
    the lock.  Re-checked the lock order: none of the callers of
    j1939_tp_set_rxtimeout() hold active_session_list_lock, so taking it for
    the rearm does not introduce a new ordering.
  - [2/4] uses J1939_XTP_ABORT_BAD_EDPO_OFFSET for the receiver, which is
    what the comment next to the definition describes and which had no
    callers; J1939_XTP_ABORT_ECTS_TOO_BIG is left to [4/4].
  - Revision history moved below the --- line, Author and Signed-off-by now
    match, and Closes: replaces Link: after Reported-by:.

Bounding the CTS packet number is [4/4] rather than part of [2/4], because
it addresses a different input and can be reviewed on its own.

Chaosheng Qu (4):
  can: j1939: arm the abort timer under the session list lock
  can: j1939: validate the EDPO packet number
  can: j1939: drop the message when the reassembled skb is gone
  can: j1939: reject a CTS that names more packets than the message has

 net/can/j1939/transport.c | 84 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 79 insertions(+), 5 deletions(-)

[PATCH net v7 1/4] can: j1939: arm the abort timer under the session list lock

[Chaosheng Qu]

From 672c37cb66569c9103f28405d455c8777c81c9e0 Mon Sep 17 00:00:00 2001
From: Chaosheng Qu <quchaosheng000406@163.com>
Date: Sun, 8 Nov 2026 09:22:28 +0800
Subject: [PATCH net v7 1/4] can: j1939: arm the abort timer under the session
 list lock

j1939_session_cancel() cancels the receive timer while holding
active_session_list_lock.  hrtimer_cancel() waits for a callback that is
already running, and j1939_tp_rxtimer() takes the same lock, so a softirq
callback waiting for the lock deadlocks against the CPU that holds it.
Cancel the timer before taking the lock instead.

Cancelling outside the lock opens a second window: the cancel/get/start
sequence is no longer atomic against j1939_tp_set_rxtimeout(), which rearms
the same timer from the receive path without holding the lock.  A
concurrent
arm in that window queues a timer whose reference is never dropped, and the
session then never reaches j1939_session_destroy(), keeping both its netdev
and its j1939_priv reference alive.  Take the lock for the rearms so both
armers are serialized; hrtimer_start() does not wait for a callback, so it
is safe to call while holding it.

A session that is already in J1939_SESSION_WAITING_ABORT also gets its
timer
back.  A second unparsable frame re-enters j1939_session_cancel() after
j1939_session_timers_cancel() has killed the timer, and without a rearmed
timer nothing ever moves that session out of WAITING_ABORT, so
unregistering
the device hangs in unregister_netdevice().

The reference accounting stays balanced: one j1939_session_get() per
hrtimer_start(), dropped by j1939_session_put() in j1939_tp_rxtimer() when
the timer fires.

Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..590a04c453b5 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1161,12 +1161,38 @@ static void __j1939_session_cancel(struct j1939_session *session,
 static void j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
+	bool abort_sent = false;
+
+	/* Cancel the abort timer before taking the list lock.
+	 *
+	 * hrtimer_cancel() waits for a callback that is already running, and
+	 * j1939_tp_rxtimer() takes the same lock, so cancelling the timer while
+	 * holding active_session_list_lock can deadlock: the lock holder waits
+	 * for the callback, the callback waits for the lock.
+	 */
+	j1939_session_rxtimer_cancel(session);
+
 	j1939_session_list_lock(session->priv);
 
 	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);
+		abort_sent = true;
+	}
+
+	/* Re-arm under the lock, whether the abort frame just went out or the
+	 * session is already in WAITING_ABORT and only needs its timer back.
+	 *
+	 * hrtimer_start() does not wait for a running callback, so arming here
+	 * cannot deadlock, and taking the lock closes the window against
+	 * j1939_tp_set_rxtimeout(), which rearms the same timer from the
+	 * receive path without holding it.
+	 */
+	if (abort_sent || session->state == J1939_SESSION_WAITING_ABORT) {
+		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);
-- 
2.43.0

[PATCH net v7 3/4] can: j1939: drop the message when the reassembled skb is gone

[Chaosheng Qu]

From ba4643113b5f6ad2b53b98dae1c25d5465d4d0c0 Mon Sep 17 00:00:00 2001
From: Chaosheng Qu <quchaosheng000406@163.com>
Date: Sun, 8 Nov 2026 09:22:29 +0800
Subject: [PATCH net v7 3/4] can: j1939: drop the message when the reassembled
 skb is gone

j1939_session_completed() takes the skb for the completed message and hands
it to j1939_sk_recv() without checking the result.  j1939_session_skb_get()
returns NULL when the offset it computes lands past the end of the queue.

The offset is "pkt.dpo * 7", and pkt.dpo is bounded when an EDPO is
received,
so this should not happen.  Guard it anyway: a NULL dereference in the
receive path is a kernel panic, while a dropped message is a lost datagram.
j1939_simple_txnext(), the only other caller of j1939_session_skb_get(),
already checks for NULL.

Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index b0e62822b56e..5f05c15114ce 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1269,9 +1269,19 @@ 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.  The only other caller of
+		 * j1939_session_skb_get(), j1939_simple_txnext(), already does
+		 * this.
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
-- 
2.43.0

[PATCH net v7 2/4] can: j1939: validate the EDPO packet number

[Chaosheng Qu]

From 4fca2aa63eeaefe7f14a6b1c5b9a3b67a2439c98 Mon Sep 17 00:00:00 2001
From: Chaosheng Qu <quchaosheng000406@163.com>
Date: Sun, 8 Nov 2026 09:22:29 +0800
Subject: [PATCH net v7 2/4] can: j1939: validate the EDPO packet number

ETP.CM_DPO is peer controlled and its packet number is accepted without any
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 still completes normally in that case.  With
pkt.dpo == pkt.total, a closing ETP.DT frame with dat[0] == 0 evaluates to
packet pkt.total - 1, which is 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" -- one full packet past the end of the queued skb --
gets NULL, and passes it straight to j1939_sk_recv(), which dereferences
oskb->sk.

Reject an EDPO that names a packet at or past pkt.total and abort the
session.  The check has to be ">=" 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 rejects a bad offset with
J1939_XTP_ABORT_BAD_EDPO_OFFSET, which the comment next to the definition
describes as "Bad EDPO offset" and which had no callers.  A transmitter
only
builds an EDPO from pkt.tx_acked, which comes from the responder's
ETP.CM_CTS, so an out-of-range value there is an oversized CTS and is
reported as J1939_XTP_ABORT_ECTS_TOO_BIG.

Bounding the CTS packet number in j1939_xtp_rx_cts_one() is a separate
change; this one only stops the value from being used as an offset.

Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 590a04c453b5..b0e62822b56e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1849,14 +1849,43 @@ 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);
+
+	/* ETP.CM_DPO 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
+	 * beyond the reassembled buffer.  The transfer still completes: with
+	 * pkt.dpo == pkt.total a closing ETP.DT with dat[0] == 0 evaluates to
+	 * the last in-order packet, so pkt.rx reaches pkt.total and the receiver
+	 * emits its own EOMA.  The loopback of that EOMA runs
+	 * j1939_session_completed(), which asks j1939_session_skb_get() for a
+	 * packet past the end of the queued skb, gets NULL, and passes it to
+	 * j1939_sk_recv().
+	 *
+	 * pkt.total is a packet count, so the last valid packet is
+	 * pkt.total - 1 and the check has to be ">=".
+	 */
+	if (dpo >= session->pkt.total) {
+		if (session->transmission) {
+			/* A transmitter only builds an EDPO from pkt.tx_acked,
+			 * which comes from the responder's ETP.CM_CTS, so an
+			 * out-of-range value there is an oversized CTS.
+			 */
+			j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
+		} else {
+			j1939_session_cancel(session, 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);
 
-- 
2.43.0

[PATCH net v7 4/4] can: j1939: reject a CTS that names more packets than the message has

[Chaosheng Qu]

From d4e65724a76b0d8570b7e4603948219f11128999 Mon Sep 17 00:00:00 2001
From: Chaosheng Qu <quchaosheng000406@163.com>
Date: Sun, 8 Nov 2026 09:22:30 +0800
Subject: [PATCH net v7 4/4] can: j1939: reject a CTS that names more packets
 than the message has

j1939_xtp_rx_cts_one() rejects a zero packet number and a window above
pkt.block, but not a packet number above pkt.total.  It then stores
"pkt - 1" in pkt.tx_acked, and the transmitter reports that value back in
an
ETP.CM_DPO.

That value is what j1939_xtp_rx_dpo_one() now bounds, so an oversized CTS
is
currently caught one step later and reported as a bad EDPO offset.  Reject
it where it originates instead, with the abort code that describes it:
J1939_XTP_ABORT_ECTS_TOO_BIG, "ECTS requested packets exceeds message
size".

Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 5f05c15114ce..6cb3dec49f3f 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1504,6 +1504,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
 		goto out_session_cancel;
 	else if (dat[1] > session->pkt.block /* 0xff for etp */)
 		goto out_session_cancel;
+	else if (pkt > session->pkt.total) {
+		/* The responder asks for more packets than the message has.
+		 * pkt - 1 becomes pkt.tx_acked below, and the transmitter
+		 * reports that value in an ETP.CM_DPO, where it would name a
+		 * packet past the end of the message.
+		 */
+		err = J1939_XTP_ABORT_ECTS_TOO_BIG;
+		goto out_session_cancel;
+	}
 
 	/* set packet counters only when not CTS(0) */
 	session->pkt.tx_acked = pkt - 1;
-- 
2.43.0

Re: [PATCH net v7 1/4] can: j1939: arm the abort timer under the session list lock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: race in j1939_session_cancel() double-arms rxtimer
  and leaks session
- [Medium] can: j1939: j1939_session_cancel() leaves rxtimer armed on
  deactivated session
- [Low] can: j1939: missing Fixes trailer for commit d966635b384b
--

commit d2f1f6071a930db192083744d44435260ec61c4f
Author: Chaosheng Qu <quchaosheng000406@163.com>

> can: j1939: arm the abort timer under the session list lock
>
> j1939_session_cancel() cancels the receive timer while holding
> active_session_list_lock.  hrtimer_cancel() waits for a callback that is
> already running, and j1939_tp_rxtimer() takes the same lock, so a softirq
> callback waiting for the lock deadlocks against the CPU that holds it.
> Cancel the timer before taking the lock instead.
>
> Cancelling outside the lock opens a second window: the cancel/get/start
> sequence is no longer atomic against j1939_tp_set_rxtimeout(), which rearms
> the same timer from the receive path without holding the lock.  A
> concurrent
> arm in that window queues a timer whose reference is never dropped, and the
> session then never reaches j1939_session_destroy(), keeping both its netdev
> and its j1939_priv reference alive.  Take the lock for the rearms so both
> armers are serialized; hrtimer_start() does not wait for a callback, so it
> is safe to call while holding it.
>
> A session that is already in J1939_SESSION_WAITING_ABORT also gets its
> timer
> back.  A second unparsable frame re-enters j1939_session_cancel() after
> j1939_session_timers_cancel() has killed the timer, and without a rearmed
> timer nothing ever moves that session out of WAITING_ABORT, so
> unregistering
> the device hangs in unregister_netdevice().
>
> The reference accounting stays balanced: one j1939_session_get() per
> hrtimer_start(), dropped by j1939_session_put() in j1939_tp_rxtimer() when
> the timer fires.
>
> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
This fixes the locking and timer re-arming in j1939_session_cancel().
Should this include:

  Fixes: d966635b384b ("can: j1939: transport: make sure the aborted session will be deactivated only once")

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..590a04c453b56 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1161,12 +1161,38 @@ static void __j1939_session_cancel(struct j1939_session *session,
>  static void j1939_session_cancel(struct j1939_session *session,
>  				 enum j1939_xtp_abort err)
>  {
> +	bool abort_sent = false;
> +
> +	/* Cancel the abort timer before taking the list lock.
> +	 *
> +	 * hrtimer_cancel() waits for a callback that is already running, and
> +	 * j1939_tp_rxtimer() takes the same lock, so cancelling the timer while
> +	 * holding active_session_list_lock can deadlock: the lock holder waits
> +	 * for the callback, the callback waits for the lock.
> +	 */
> +	j1939_session_rxtimer_cancel(session);
> +
>  	j1939_session_list_lock(session->priv);
>  
>  	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);
> +		abort_sent = true;
>  	}
> +
> +	/* Re-arm under the lock, whether the abort frame just went out or the
> +	 * session is already in WAITING_ABORT and only needs its timer back.
> +	 *
> +	 * hrtimer_start() does not wait for a running callback, so arming here
> +	 * cannot deadlock, and taking the lock closes the window against
> +	 * j1939_tp_set_rxtimeout(), which rearms the same timer from the
> +	 * receive path without holding it.
> +	 */

[Severity: High]
Can a concurrent timer arm between j1939_session_rxtimer_cancel() and
hrtimer_start() leak a session reference?

j1939_tp_set_rxtimeout() does not acquire active_session_list_lock, so
taking the lock here does not serialize the two paths.  Even between two
concurrent j1939_session_cancel() callers (for example from
j1939_tp_txtimer() and j1939_tp_recv()), both cancel rxtimer before taking
the lock:

CPU0                                  CPU1
j1939_session_cancel()
  j1939_session_rxtimer_cancel()
                                      j1939_session_cancel()
                                        j1939_session_rxtimer_cancel()
  j1939_session_list_lock()
  session->state = ..._WAITING_ABORT
  j1939_session_get(session)
  hrtimer_start(&session->rxtimer)
  j1939_session_list_unlock()
                                        j1939_session_list_lock()
                                        /* state == WAITING_ABORT */
                                        j1939_session_get(session)
                                        hrtimer_start(&session->rxtimer)
                                          <-- re-queues armed timer, leaks ref

Calling hrtimer_start() on an already-queued rxtimer replaces it in place
without dropping the previous reference, so session->kref never reaches zero
and unregister_netdevice() hangs waiting for the leaked netdev reference.

> +	if (abort_sent || session->state == J1939_SESSION_WAITING_ABORT) {
> +		j1939_session_get(session);
> +		hrtimer_start(&session->rxtimer,
> +			      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> +			      HRTIMER_MODE_REL_SOFT);
> +	}

[Severity: Medium]
Does re-arming session->rxtimer when session->state is already
J1939_SESSION_WAITING_ABORT leave rxtimer running on a deactivated session in
j1939_xtp_rx_rts_session_active()?

When a duplicate RTS frame arrives on an active session with
session->last_cmd != 0, j1939_xtp_rx_rts_session_active() cancels and
immediately deactivates the session:

net/can/j1939/transport.c:j1939_xtp_rx_rts_session_active() {
    ...
		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) {
			...
			j1939_session_deactivate_activate_next(session);
		}
}

With this change, j1939_session_cancel() re-arms rxtimer, and
j1939_session_deactivate_activate_next() then sets session->state to
J1939_SESSION_DONE and removes the session from priv->active_session_list
without cancelling rxtimer.  When rxtimer fires 500ms later on the
J1939_SESSION_DONE session, j1939_tp_rxtimer() takes the else branch and
sends a duplicate J1939_ERRQUEUE_RX_ABORT notification.

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

Re: [PATCH net v7 2/4] can: j1939: validate the EDPO packet number

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: missing txtimer cancel before
  j1939_session_cancel()
- [Medium] can: j1939: session ref leak in j1939_session_cancel() on
  rxtimer race
- [Low] can: j1939: missing Fixes trailer for EDPO bounds check
--

commit 1f6785035506d85ce93ce6a036332f472884137d
Author: Chaosheng Qu <quchaosheng000406@163.com>

> can: j1939: validate the EDPO packet number
>
> ETP.CM_DPO is peer controlled and its packet number is accepted without any
> 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 still completes normally in that case.  With
> pkt.dpo == pkt.total, a closing ETP.DT frame with dat[0] == 0 evaluates to
> packet pkt.total - 1, which is 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" -- one full packet past the end of the queued skb --
> gets NULL, and passes it straight to j1939_sk_recv(), which dereferences
> oskb->sk.
>
> Reject an EDPO that names a packet at or past pkt.total and abort the
> session.  The check has to be ">=" 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 rejects a bad offset with
> J1939_XTP_ABORT_BAD_EDPO_OFFSET, which the comment next to the definition
> describes as "Bad EDPO offset" and which had no callers.  A transmitter
> only
> builds an EDPO from pkt.tx_acked, which comes from the responder's
> ETP.CM_CTS, so an out-of-range value there is an oversized CTS and is
> reported as J1939_XTP_ABORT_ECTS_TOO_BIG.
>
> Bounding the CTS packet number in j1939_xtp_rx_cts_one() is a separate
> change; this one only stops the value from being used as an offset.
>
> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
Since this fixes a NULL pointer dereference present since J1939 support was
introduced, should it include a Fixes tag:

  Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 590a04c453b56..b0e62822b56e4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1849,14 +1849,43 @@ 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);
> +
> +	/* ETP.CM_DPO 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
> +	 * beyond the reassembled buffer.  The transfer still completes: with
> +	 * pkt.dpo == pkt.total a closing ETP.DT with dat[0] == 0 evaluates to
> +	 * the last in-order packet, so pkt.rx reaches pkt.total and the receiver
> +	 * emits its own EOMA.  The loopback of that EOMA runs
> +	 * j1939_session_completed(), which asks j1939_session_skb_get() for a
> +	 * packet past the end of the queued skb, gets NULL, and passes it to
> +	 * j1939_sk_recv().
> +	 *
> +	 * pkt.total is a packet count, so the last valid packet is
> +	 * pkt.total - 1 and the check has to be ">=".
> +	 */
> +	if (dpo >= session->pkt.total) {
> +		if (session->transmission) {
> +			/* A transmitter only builds an EDPO from pkt.tx_acked,
> +			 * which comes from the responder's ETP.CM_CTS, so an
> +			 * out-of-range value there is an oversized CTS.
> +			 */
> +			j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
> +		} else {
> +			j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		}
> +		return;
> +	}

[Severity: High]
Does aborting here without calling j1939_session_timers_cancel(session) leave
session->txtimer armed?

j1939_session_cancel() only cancels session->rxtimer, which is why the other
receive-path abort handlers (j1939_xtp_rx_cts_one(),
j1939_xtp_rx_rts_session_active() and j1939_xtp_rx_dat_one()) call
j1939_session_timers_cancel(session) first.

On the transmitter path, after j1939_tp_txtimer() sends ETP.CM_DPO with an
out-of-range pkt.tx, j1939_session_tx_dat() sees NULL from
j1939_session_skb_get_by_offset() and returns -ENOBUFS, so j1939_tp_txtimer()
re-arms session->txtimer for 10..25 ms.  When the looped-back ETP.CM_DPO then
hits this check, session->txtimer stays active on the aborting session, retries
up to J1939_XTP_TX_RETRY_LIMIT and overwrites session->err with -ENETUNREACH
(or on a receiver session, transmits CTS or EOMA and clobbers the abort
timeout via j1939_tp_set_rxtimeout()).

[Severity: Medium]
Can calling j1939_session_cancel() here race with a second cancel or timeout
update to double-arm session->rxtimer and leak a struct j1939_session
reference?

In j1939_session_cancel() (modified in commit "can: j1939: arm the abort timer
under the session list lock"), j1939_session_rxtimer_cancel() runs before
taking active_session_list_lock, and then hrtimer_start(&session->rxtimer, ...)
runs unconditionally under the lock whenever abort_sent is true or
session->state == J1939_SESSION_WAITING_ABORT:

CPU0                              CPU1
j1939_xtp_rx_dpo_one()            j1939_xtp_rx_dpo_one()
  j1939_session_cancel()            j1939_session_cancel()
    j1939_session_rxtimer_cancel()    j1939_session_rxtimer_cancel()
    j1939_session_list_lock()
    __j1939_session_cancel()
      state = WAITING_ABORT
    j1939_session_get()
    hrtimer_start(&rxtimer, ...)
    j1939_session_list_unlock()
                                      j1939_session_list_lock()
                                      /* state == WAITING_ABORT */
                                      j1939_session_get()
                                      hrtimer_start(&rxtimer, ...)
                                        <-- overwrites armed timer, leaks ref

j1939_tp_set_rxtimeout() also rearms session->rxtimer without holding
active_session_list_lock, so it can likewise arm session->rxtimer between
j1939_session_rxtimer_cancel() and hrtimer_start().  The leaked kref prevents
j1939_session_destroy() from running, leaking the session and its tracked
net_device reference and hanging unregister_netdevice().

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