[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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 4

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).

- [High] The commit message says the rearms take the list lock "so both
  armers are serialized".
- [High] This is a new regression: two j1939_session_cancel() calls
  running at the same time on the same session now double-arm the…
- [Medium] j1939_session_cancel() now also re-arms the rxtimer when the
  state is already WAITING_ABORT.
- [Low] The patch fixes a deadlock (hrtimer_cancel() under
  active_session_list_lock against j1939_tp_rxtimer(), which takes that…
- [Low] The subject, 'can: j1939: arm the abort timer under the session
  list lock', does not describe the change.
- [Low] This behaviour change is not described in the commit message.
- [Low] The flag abort_sent and the comment 'whether the abort frame just
  went out or the session is already in WAITING_ABORT' in…

--

can: j1939: arm the abort timer under the session list lock

This patch moves the rxtimer cancel in j1939_session_cancel() outside of
active_session_list_lock, to avoid a deadlock against j1939_tp_rxtimer().
It then re-arms the abort timer under the lock. It also re-arms the timer
for sessions that are already in J1939_SESSION_WAITING_ABORT.

> can: j1939: arm the abort timer under the session list lock

[Severity: Low]
This isn't a bug, but does the subject describe the change? Before this
patch, j1939_session_cancel() already armed the abort timer under the lock,
because j1939_tp_set_rxtimeout() was called inside
j1939_session_list_lock().

The patch makes two changes. It moves the cancel out of the lock, which is
the deadlock fix. It also re-arms WAITING_ABORT sessions, which is the
stuck-session fix.

Would these be easier to bisect and backport as two separate patches?

[ ... ]

> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? git blame shows that
j1939_session_cancel(), with j1939_tp_set_rxtimeout() called inside the
lock, comes from commit d966635b384b.

The related fix 1809c82aa073 for the same session leak class carried
"Fixes: 9d71dd0c7009" and "Cc: stable@vger.kernel.org".

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

[Severity: Low]
This isn't a bug, but the name abort_sent doesn't quite match the code.
__j1939_session_cancel() skips j1939_xtp_tx_abort() for broadcast
sessions:

net/can/j1939/transport.c:__j1939_session_cancel() {
    ...
	session->state = J1939_SESSION_WAITING_ABORT;
	/* do not send aborts on incoming broadcasts */
	if (!j1939_cb_is_broadcast(&session->skcb)) {
    ...
}

abort_sent is still set to true in that case. __j1939_session_cancel()
also always sets J1939_SESSION_WAITING_ABORT, so the state check below is
already true whenever abort_sent is.

Is the flag needed at all? The comment below ("whether the abort frame just
went out or the session is already in WAITING_ABORT") describes two cases
that the code never distinguishes.

> +
> +	/* 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]
Does taking the lock here actually close the window against
j1939_tp_set_rxtimeout()? That function is unchanged, and it still does
cancel, get and start without active_session_list_lock:

net/can/j1939/transport.c: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);
}

None of its callers hold the list lock either. That covers the RX softirq
paths and the txtimer paths: j1939_session_tx_rts(),
j1939_session_tx_dpo(), j1939_session_tx_dat(), j1939_session_tx_cts(),
j1939_session_tx_eoma() and j1939_simple_txnext().

Only one of the two armers takes the lock. Are they really serialized, as
the commit message says ("so both armers are serialized")?

The cancel is now outside the lock. Can this sequence leak a session
reference?

CPU A (j1939_session_cancel)       CPU B (j1939_tp_set_rxtimeout)
j1939_session_rxtimer_cancel()
                                   j1939_session_rxtimer_cancel()
                                     (nothing pending)
                                   j1939_session_get()
                                   hrtimer_start()
j1939_session_list_lock()
__j1939_session_cancel()
j1939_session_get()
hrtimer_start()
  (re-enqueues the already queued timer)

Both sides take a reference, but only one timer is queued.
j1939_tp_rxtimer() calls j1939_session_put() once per expiry. The reverse
order leaks the same way.

Once a kref leaks, j1939_session_destroy() never runs. So netdev_put(),
j1939_priv_put() and, for TX sessions, the sock_put() in
__j1939_session_drop() never happen either. Wouldn't unregister_netdevice()
then hang, which is the leak the commit message says this patch closes?

j1939_cancel_active_session() at unregister doesn't look able to recover
this. It only drops one rxtimer reference through hrtimer_try_to_cancel(),
plus the list reference.

Would it help to use hrtimer_try_to_cancel() under the lock and put when it
returns 1, as j1939_cancel_active_session() does? Another option is to tie
the get to whether the timer was actually inactive.

> +	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: High]
Can two concurrent j1939_session_cancel() calls on the same session
double-arm the rxtimer here? For example, CPU A is in
j1939_tp_txtimer()->j1939_session_cancel(). CPU B is in an RX
out_session_cancel path such as j1939_xtp_rx_dat_one() or
j1939_xtp_rx_cts_one():

CPU A                                CPU B
j1939_session_rxtimer_cancel()
  hrtimer_cancel() returns 1, put
                                     j1939_session_rxtimer_cancel()
                                       (nothing pending)
j1939_session_list_lock()
__j1939_session_cancel()
  state = WAITING_ABORT
j1939_session_get()
hrtimer_start()
j1939_session_list_unlock()
                                     j1939_session_list_lock()
                                     state == WAITING_ABORT
                                     j1939_session_get()
                                     hrtimer_start()
                                       (re-enqueues the queued timer)
                                     j1939_session_list_unlock()

That is two gets for a single expiry of j1939_tp_rxtimer(), so one kref
would never be dropped.

Before this patch, CPU B saw WAITING_ABORT under the lock and did nothing.
The WAITING_ABORT re-arm combined with the lockless cancel seems to be what
introduces this.

The commit message says "The reference accounting stays balanced: one
j1939_session_get() per hrtimer_start()". Doesn't that only hold if
hrtimer_start() is never called on a timer that is already queued?

[Severity: Medium]
Does the WAITING_ABORT re-arm conflict with the existing workaround in
j1939_xtp_rx_rts_session_active()? That code still assumes
j1939_session_cancel() won't restart the timer in WAITING_ABORT:

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) {
			/* 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);
		}
    ...
}

Take a receive session where the peer sends a second RTS. The first
j1939_session_timers_cancel() and j1939_session_cancel() pair at the top of
this function moves the session to WAITING_ABORT and arms the timer.

The second pair then kills that timer. With this patch,
j1939_session_cancel() re-arms it with a new reference.
j1939_session_deactivate_activate_next() then moves the session to DONE
without cancelling the timer.

When that timer fires on the DONE session, j1939_tp_rxtimer() takes the
generic else branch. The state check fails, but it still runs:

		if (!session->sk)
			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);

Doesn't this deliver an extra J1939_ERRQUEUE_RX_ABORT to matching sockets
for a session that has already finished? It also keeps the session, netdev
and priv references alive for another J1939_XTP_ABORT_TIMEOUT_MS.

The comment in j1939_xtp_rx_rts_session_active() would also no longer
match what j1939_session_cancel() does.

[Severity: Low]
Is this change in timeout semantics intended? Every j1939_session_cancel()
on a session already in WAITING_ABORT now cancels the pending abort timer
and starts a fresh J1939_XTP_ABORT_TIMEOUT_MS timer.

That includes the calls from j1939_tp_txtimer(), which don't cancel the
timers first:

		if (session->skcb.addr.type != J1939_SIMPLE) {
			j1939_session_cancel(session, J1939_XTP_ABORT_OTHER);

Before this patch, those calls did nothing and the original abort deadline
stood. Now the forced deactivation happens 500ms after the last cancel,
not 500ms after the abort.

A peer could keep sending malformed TP frames less than 500ms apart, for
example DT frames with skb->len != 8 that reach out_session_cancel in
j1939_xtp_rx_dat_one(). Wouldn't that keep the session in WAITING_ABORT on
the active list indefinitely?

The commit message only says such sessions "also gets its timer back".
Could it mention that an existing pending deadline is also pushed back?

>  
>  	j1939_session_list_unlock(session->priv);

[ ... ]

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new NULL branch in j1939_session_completed()
  (net/can/j1939/transport.c) drops the reassembled message without…
- [Low] This patch makes the comment in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c, lines 1871-1883) out of date.

--

can: j1939: drop the message when the reassembled skb is gone

This adds a NULL check in j1939_session_completed() on the skb returned by
j1939_session_skb_get(). A failed lookup now drops the reassembled message
instead of passing NULL to j1939_sk_recv(). The commit message says
pkt.dpo is already bounded when an EDPO is received, so this is defense in
depth.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index b0e62822b56e4..5f05c15114ce4 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);
> +		}

[Severity: Low]
If the offset arithmetic does regress, would anyone notice?

When se_skb is NULL, the message is dropped with no WARN. session->err
stays at zero, and nothing is queued for userspace. The only output is the
netdev_dbg() in j1939_session_skb_get_by_offset(), which is off unless
dynamic debug is enabled.

On the userspace side, j1939_session_destroy() only reports RX_ABORT when
session->err is set:

	} else if (session->err) {
			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
	}

A receiver using SOF_TIMESTAMPING_RX_SOFTWARE would see
J1939_ERRQUEUE_RX_RTS, and possibly RX_DPO. After that it would get
neither the data nor an RX_ABORT.

The commit message also says:

    a dropped message is a lost datagram.

For an RTS/CTS receive session, j1939_session_completed() is reached from
j1939_xtp_rx_eoma_one() when the receiver's own EOMA comes back on
loopback. By then the originator has already been told the message was
delivered, so the two sides would disagree about delivery.

This branch cannot be reached in the current tree.
j1939_xtp_rx_dpo_one() rejects dpo >= session->pkt.total before storing
it, and TP and BAM sessions keep pkt.dpo at 0. The concern is that the
guard would not catch the regression it was added for.

Would WARN_ON_ONCE(!se_skb) plus setting session->err (for example to
-EBADMSG) fit better here? That would show the condition in the logs and
deliver the RX_ABORT errqueue notification.

[Severity: Low]
This isn't a bug, but this change makes a comment in
j1939_xtp_rx_dpo_one() out of date. That comment was added by "can: j1939:
validate the EDPO packet number" and still says:

	 * 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().

With the new if (se_skb) check, a failed lookup no longer reaches
j1939_sk_recv(). Without the EDPO bound, the message would now be
silently dropped after the receiver's EOMA has been sent.

Could that comment be updated along with this change? The last patch in
the series, "can: j1939: reject a CTS that names more packets than the
message has", only touches j1939_xtp_rx_cts_one(). The comment is still
stale at the end of the series.

[ ... ]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] The new EDPO rejection path in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c) calls j1939_session_cancel() without first…
- [Low] The commit fixes a NULL pointer dereference that any remote CAN
  node can trigger, but it carries no Fixes: tag and no Cc: stable.

--

can: j1939: validate the EDPO packet number

This patch makes j1939_xtp_rx_dpo_one() reject an ETP.CM_DPO whose packet
number is at or past pkt.total, and abort the session when that happens.
A receiver aborts with J1939_XTP_ABORT_BAD_EDPO_OFFSET. A transmitter aborts
with J1939_XTP_ABORT_ECTS_TOO_BIG.

>     Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
Should this commit carry a Fixes: tag and Cc: stable?

The commit message describes a NULL pointer dereference that a remote CAN
node can trigger:

j1939_session_completed()
    se_skb = j1939_session_skb_get(session);   <- NULL
    j1939_sk_recv(session->priv, se_skb);      <- dereferences oskb->sk

git blame on the baseline shows that j1939_xtp_rx_dpo_one() and
j1939_xtp_rx_dat_one() come from commit 9d71dd0c7009 ("can: add support of
SAE J1939 protocol"). Would a Fixes: tag for that commit be appropriate?

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

[ ... ]

> +	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: Medium]
Should this branch call j1939_session_timers_cancel() before
j1939_session_cancel()? The other receive path aborts in this file do:

j1939_xtp_rx_cts_one() {
    ...
 out_session_cancel:
	j1939_session_timers_cancel(session);
	j1939_session_cancel(session, err);
}

j1939_xtp_rx_dat_one() and both branches of
j1939_xtp_rx_rts_session_active() follow the same pattern.

j1939_session_cancel() only cancels and re-arms the rxtimer. That means the
txtimer can still be pending after this abort. If the txtimer is already
running on another CPU, nothing waits for it to finish.

On a receiver session, a remote node can send ETP.CM_RTS. In response,
j1939_xtp_rx_rts() calls j1939_tp_schedule_txtimer(session, 0) to send the
CTS. Suppose the node then immediately sends an ETP.CM_DPO with a packet
number >= pkt.total. j1939_xtp_rx_dpo_one() does not check last_cmd, so the
frame reaches this branch.

This branch returns before last_cmd is updated, so last_cmd is still
J1939_ETP_CMD_RTS. When the txtimer then runs:

j1939_tp_txtimer()
    j1939_xtp_txnext_receiver()
        case J1939_ETP_CMD_RTS:
            j1939_session_tx_cts()
                j1939_tp_tx_ctl(session, true, dat);
                j1939_tp_set_rxtimeout(session, 1250);

Does this send a CTS after the abort has already gone out? Does it also
replace the abort timeout with a 1250ms rx timeout?

j1939_tp_set_rxtimeout() does not take active_session_list_lock. Suppose it
runs at the same time as the cancel/get/hrtimer_start sequence in
j1939_session_cancel(). Could the rxtimer then be armed twice with two
session references taken, leaking one of them? A leaked reference would
keep the j1939_priv and netdev references alive, and could hang
unregister_netdevice().

The transmitter branch looks like it has the same gap. j1939_tp_txtimer()
does not check session->state. On -ENOBUFS it keeps re-arming itself. At the
retry limit it does this without the lock:

	session->err = -ENETUNREACH;
	j1939_session_rxtimer_cancel(session);
	j1939_session_deactivate_activate_next(session);

That overwrites the abort error and cancels the abort timer.
j1939_tp_cmd_recv() sends a frame down the transmitter path whenever
j1939_tp_im_transmitter(skcb) is true. Can a frame with a spoofed local SA
still reach this branch after the CTS bound is added?

Would it help to add j1939_session_timers_cancel(session) before
j1939_session_cancel() here? hrtimer_cancel() waits for a running callback,
so that would close the window.

> +
>  	/* 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);

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] The commit message of "can: j1939: reject a CTS that names
  more packets than the message has" gets the current behaviour wrong…
- [Low] This patch makes the transmitter-role branch of
  j1939_xtp_rx_dpo_one() (net/can/j1939/transport.c, lines 1894-1899)…

--

can: j1939: reject a CTS that names more packets than the message has

This patch adds a check to j1939_xtp_rx_cts_one(). A CTS whose packet
number is above pkt.total is now rejected, and the session is aborted
with J1939_XTP_ABORT_ECTS_TOO_BIG. Before this, the function only
rejected a zero packet number and a window above pkt.block.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 5f05c15114ce4..6cb3dec49f3f7 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;
> +	}

[Severity: Medium]
Does the commit message describe the current behaviour correctly? It
says:

  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.

Before this patch, the host that receives an oversized ETP CTS goes
through this path:

j1939_xtp_rx_cts_one()
    session->pkt.tx_acked = pkt - 1;   /* >= pkt.total */
j1939_xtp_txnext_transmiter()
    j1939_session_tx_dpo()
        dat[1] = session->pkt.last - session->pkt.tx_acked; /* 0 or wraps */
local echo of that DPO -> j1939_tp_cmd_recv()
    j1939_xtp_rx_dpo(priv, skb, true)
        j1939_xtp_rx_dpo_one()

In j1939_xtp_rx_dpo_one(), the echo then takes the transmitter branch:

	if (dpo >= session->pkt.total) {
		if (session->transmission) {
			...
			j1939_session_cancel(session, J1939_XTP_ABORT_ECTS_TOO_BIG);
		} else {
			j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
		}

So isn't the abort code already ECTS_TOO_BIG, not a bad EDPO offset?
The earlier commit in this series, "can: j1939: validate the EDPO
packet number", says the same. For ETP, the actual change seems to be
that the out-of-range DPO, with a zero or wrapped dat[1], is no longer
sent on the bus.

The new check also runs for plain TP sessions. There, pkt = dat[2] and
there is no DPO step. Before this patch, an oversized TP CTS reached
j1939_session_tx_dat() with pkt.tx >= pkt.total:

	se_skb = j1939_session_skb_get_by_offset(session, session->pkt.tx * 7);
	if (!se_skb)
		return -ENOBUFS;

The txtimer then retried until one of two things happened:

  - the rxtimer aborted the session with J1939_XTP_ABORT_TIMEOUT, or
  - the retry limit ended the session with -ENETUNREACH.

With this patch, a TP session aborts at once with code 15. That code is
documented as "ECTS requested packets exceeds message size".

Could the commit message and the new comment cover the TP case as well?
The peer and the socket user will see this change. The comment
currently explains the check only through ETP.CM_DPO.

>  
>  	/* set packet counters only when not CTS(0) */
>  	session->pkt.tx_acked = pkt - 1;

[Severity: Low]
With this check in place, does the transmitter branch in
j1939_xtp_rx_dpo_one() still match its comment?

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

After this patch, pkt.tx_acked is at most pkt.total - 1 here. The one
exception is j1939_xtp_rx_eoma_one(), which sets it to pkt.total just
before the session completes. j1939_session_tx_dpo() copies
pkt.tx_acked into the DPO packet number, so a DPO built by the local
transmitter can no longer reach dpo >= pkt.total.

The only frame that can still reach this branch is an ETP.CM_DPO with a
local SA that the local transmitter did not build. One example is
another node on the bus using the same SA. Another is a raw CAN frame
injected on the interface. j1939_tp_cmd_recv() passes such a frame to
j1939_xtp_rx_dpo(priv, skb, true).

For that frame, wouldn't J1939_XTP_ABORT_BAD_EDPO_OFFSET be the more
accurate abort reason? The commit message refers to the bound in
j1939_xtp_rx_dpo_one(). Should that role split and its comment be
updated as part of this patch?

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