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

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

[Chaosheng Qu]

This series fixes two bugs in the j1939 transport layer, plus two small
hardening changes that fall out of them.

The first bug is a timer reference leak.  The receive timer is cancelled and
restarted from three places, and they do not agree on
active_session_list_lock: the receive handlers call the helper without the
lock, j1939_session_cancel() called it with the lock held, and the rx timeout
branch of j1939_tp_rxtimer() open-coded the same sequence.  Two callers can
interleave between the cancel and the start, and the second hrtimer_start()
reprograms an already armed timer without dropping the reference the first
caller took, so the session and its netdev_hold() reference leak.  [1/4]
replaces the three copies with one helper that cancels with
hrtimer_try_to_cancel() under the lock, so the cancel and the start are one
step.

The second bug is a NULL dereference driven by a peer.  ETP.CM_DPO is peer
controlled and its packet number is not validated; a value at or past
pkt.total moves pkt.dpo past the reassembled buffer, the transfer still
completes, and the local loopback of the receiver's own EOMA hands a NULL
skb to j1939_sk_recv(), which dereferences oskb->sk.  [2/4] rejects that
EDPO.  [3/4] checks the skb in j1939_session_completed() anyway, so a future
regression in the offset arithmetic degrades to a dropped message instead of
a panic.  [4/4] rejects an oversized CTS where it is parsed, instead of
letting it come back one step later as a bad EDPO offset.

Testing: [2/4] was verified on a local vcan link -- ETP.CM_RTS with a size of
1786 bytes (256 packets), then ETP.CM_DPO packet 256 and an ETP.DT with
sequence 0, dereferenced a NULL skb 3/3 times before the bound check and was
rejected 3/3 times after it.  The seq 0 case is the one that made
j1939_xtp_rx_dat_one() compute the last in-order packet, so the transfer
completed and the receiver's own EOMA came back through the loopback, as
described in [2/4].  For the timer side: two unparsable ETP.DT frames
followed by "ip link del" left unregister_netdevice() waiting for the session
reference; with the abort timer rearmed the session is deactivated about
500 ms after the second frame.  Each patch builds standalone (net/can/j1939
built in, x86_64) and passes scripts/checkpatch.pl with no errors or
warnings.  I have no j1939 hardware, so none of this ran on a real CAN bus,
and the race in [1/4] is argued from the lock ordering rather than caught in
the act.

Changes in v9:

- Send the patches as replies to the cover letter; in v8 they went out as
  four separate threads.
- Start the first sentence of every changelog in the imperative.
- Add the Assisted-by tag required by
  Documentation/process/coding-assistants.rst, which v8 was missing.
- Add the missing Fixes tags and Cc: stable for [1/4] and [4/4]; all four
  patches fix behaviour that dates from the original J1939 commit.
- Say in [2/4] where the CTS bound lives, instead of calling it a separate
  fix while [4/4] of the same series does it.
- Give this cover letter an overview of the series and this changelog.

Functionally the patches are unchanged since v8; the only code text that
changed is one comment in [2/4].

Changes in v8:

- [1/4] rewritten: the v7 version armed the timer after cancelling it outside
  the lock, which left the race it was meant to close.
- [2/4] cancels the session timers before aborting, and carries the Fixes tag
  for the NULL dereference.
- [3/4] and [4/4] unchanged from v7.

---
Chaosheng Qu (4):
      can: j1939: serialize cancel and rearm of the receive timer
      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 | 119 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 97 insertions(+), 22 deletions(-)
---
base-commit: 2f2f7eeef8b0a1a9b9ca998231419873ccae8d4e
change-id: 20261111-j1939-edpo-v9-daeeb60af993

Best regards,
--  
Chaosheng Qu <quchaosheng000406@163.com>

[PATCH net v9 1/4] can: j1939: serialize cancel and rearm of the receive timer

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Cancel and rearm the receive timer as one atomic step under
active_session_list_lock.  j1939_tp_set_rxtimeout() cancels the timer,
takes a reference and starts it again, but its callers do not agree on that
lock: the receive handlers call it without the lock, while
j1939_session_cancel() is reached from the transmit timer path and used to
call it with the lock held.

That makes the sequence non-atomic.  If one caller starts the timer after
another has cancelled it and before that one starts it again, the second
hrtimer_start() reprograms an already armed timer.  hrtimer_start() does
not report that the timer was armed and does not drop the reference the
first caller took, so that reference is never released: the session, and
the netdev_hold() reference it carries, leak.

The lock cannot simply be taken around the existing helper, because
hrtimer_cancel() waits for a running callback and j1939_tp_rxtimer() takes
the same lock.  Add j1939_tp_arm_rxtimer_locked(), which uses
hrtimer_try_to_cancel() instead: it never waits, so it is safe under the
lock, and it returns 1 only when the timer was queued, which is the only
case in which the caller's reference has to be dropped.

j1939_tp_set_rxtimeout() now holds the lock around the helper,
j1939_session_cancel() calls the helper instead of hand-rolling
get + hrtimer_start(), and the rx timeout branch of j1939_tp_rxtimer()
does the same, removing a second copy of that sequence.

Every existing caller of j1939_tp_set_rxtimeout() is outside the lock, so
the added acquisition does not recurse.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 66 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5..fa23260e1 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -721,15 +721,42 @@ void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
 		      HRTIMER_MODE_REL_SOFT);
 }
 
-static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
-					  int msec)
+/*
+ * Rearm the receive timer as one step under active_session_list_lock.
+ *
+ * j1939_tp_set_rxtimeout() is called from receive handlers that do not hold
+ * the session list lock, while j1939_session_cancel() is called from the
+ * transmit timer path.  If the cancel and the rearm are not serialized, a
+ * concurrent caller can start the timer after this one cancelled it and
+ * before this one starts it again: hrtimer_start() on an already armed timer
+ * reprograms it without dropping the reference taken by that caller, and the
+ * session leaks together with its netdev reference.
+ *
+ * hrtimer_try_to_cancel() does not wait for a running callback, so it can be
+ * called with the lock held even though j1939_tp_rxtimer() takes the same
+ * lock; a callback that is already running drops its own reference.  For the
+ * same reason the return value is only 1 when the timer was queued: when this
+ * runs from j1939_tp_rxtimer() itself, the timer is no longer queued and 0 is
+ * the expected result.
+ */
+static void j1939_tp_arm_rxtimer_locked(struct j1939_session *session, int msec)
 {
-	j1939_session_rxtimer_cancel(session);
+	if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
+		j1939_session_put(session);
+
 	j1939_session_get(session);
 	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
 		      HRTIMER_MODE_REL_SOFT);
 }
 
+static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
+					  int msec)
+{
+	j1939_session_list_lock(session->priv);
+	j1939_tp_arm_rxtimer_locked(session, msec);
+	j1939_session_list_unlock(session->priv);
+}
+
 static int j1939_session_tx_rts(struct j1939_session *session)
 {
 	u8 dat[8];
@@ -1161,18 +1188,29 @@ 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;
+
 	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);
+		j1939_tp_arm_rxtimer_locked(session, J1939_XTP_ABORT_TIMEOUT_MS);
 	}
 
 	j1939_session_list_unlock(session->priv);
 
 	if (!session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+
+	/* The abort already went out on an earlier call, so this one armed no
+	 * timer.  Several callers cancel the timers before calling here, which
+	 * would otherwise leave the session on the list with nothing to finish
+	 * it off, so deactivate it now.
+	 */
+	if (waiting)
+		j1939_session_deactivate_activate_next(session);
 }
 
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
@@ -1279,10 +1317,8 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
 		    session->state < J1939_SESSION_ACTIVE_MAX) {
 			netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
 				     __func__, session);
-			j1939_session_get(session);
-			hrtimer_start(&session->rxtimer,
-				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
-				      HRTIMER_MODE_REL_SOFT);
+			j1939_tp_arm_rxtimer_locked(session,
+						    J1939_XTP_ABORT_TIMEOUT_MS);
 			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
 		}
 		j1939_session_list_unlock(session->priv);
@@ -1729,16 +1765,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;
 	}

-- 
2.43.0

Re: [PATCH net v9 1/4] can: j1939: serialize cancel and rearm of the receive timer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] can: j1939: session ref leak in j1939_tp_arm_rxtimer_locked()
- [Medium] can: j1939: armed rxtimer left on deactivated session in
  cancel paths
- [Medium] can: j1939: race on session->state in j1939_tp_rxtimer()
--

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

can: j1939: serialize cancel and rearm of the receive timer

This commit introduces j1939_tp_arm_rxtimer_locked() to cancel and rearm
session->rxtimer under active_session_list_lock using
hrtimer_try_to_cancel(), and updates session cancellation and RTS handling
paths.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..fa23260e1833f 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -721,15 +721,42 @@ void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
>  		      HRTIMER_MODE_REL_SOFT);
>  }
>  
> -static inline void j1939_tp_set_rxtimeout(struct j1939_session *session,
> -					  int msec)
> +/*
> + * Rearm the receive timer as one step under active_session_list_lock.
> + *
> + * j1939_tp_set_rxtimeout() is called from receive handlers that do not hold
> + * the session list lock, while j1939_session_cancel() is called from the
> + * transmit timer path.  If the cancel and the rearm are not serialized, a
> + * concurrent caller can start the timer after this one cancelled it and
> + * before this one starts it again: hrtimer_start() on an already armed timer
> + * reprograms it without dropping the reference taken by that caller, and the
> + * session leaks together with its netdev reference.
> + *
> + * hrtimer_try_to_cancel() does not wait for a running callback, so it can be
> + * called with the lock held even though j1939_tp_rxtimer() takes the same
> + * lock; a callback that is already running drops its own reference.  For the
> + * same reason the return value is only 1 when the timer was queued: when this
> + * runs from j1939_tp_rxtimer() itself, the timer is no longer queued and 0 is
> + * the expected result.
> + */
> +static void j1939_tp_arm_rxtimer_locked(struct j1939_session *session, int msec)
> +{
> +	if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
> +		j1939_session_put(session);
> +
>  	j1939_session_get(session);
>  	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
>  		      HRTIMER_MODE_REL_SOFT);
>  }

[Severity: High]
Can j1939_tp_arm_rxtimer_locked() still leak a session reference when
session->rxtimer is armed more than once while j1939_tp_rxtimer() is
running?

kernel/time/hrtimer.c:hrtimer_try_to_cancel() {
    ...
	if (!hrtimer_callback_running(timer)) {
		ret = remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE);
		if (ret)
			trace_hrtimer_cancel(timer);
	}
    ...
}

__run_hrtimer() keeps base->running set to &session->rxtimer for the entire
duration of j1939_tp_rxtimer() until after the callback returns, so
hrtimer_try_to_cancel() skips remove_hrtimer() and returns -1 even if
session->rxtimer has already been re-enqueued.

CPU0                              CPU1
j1939_tp_rxtimer()
                                  j1939_tp_set_rxtimeout()
                                    j1939_session_list_lock()
                                    j1939_tp_arm_rxtimer_locked()
                                      hrtimer_try_to_cancel() -> -1
                                      j1939_session_get(session)
                                      hrtimer_start(&session->rxtimer, ...)
                                    j1939_session_list_unlock()
  j1939_session_list_lock()
  j1939_tp_arm_rxtimer_locked()
    hrtimer_try_to_cancel() -> -1
    j1939_session_get(session)
    hrtimer_start(&session->rxtimer, ...)  <-- leaks CPU1's session ref

Because hrtimer_try_to_cancel() returns -1 on CPU0 while the callback is
running, j1939_session_put() is skipped and hrtimer_start() reprograms the
queued timer in place without releasing the first reference, permanently
leaking the session and its netdev_hold() reference.

[ ... ]

> @@ -1161,18 +1188,29 @@ 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;
> +
>  	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);
> +		j1939_tp_arm_rxtimer_locked(session, J1939_XTP_ABORT_TIMEOUT_MS);
>  	}
>  
>  	j1939_session_list_unlock(session->priv);
>  
>  	if (!session->sk)
>  		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> +
> +	/* The abort already went out on an earlier call, so this one armed no
> +	 * timer.  Several callers cancel the timers before calling here, which
> +	 * would otherwise leave the session on the list with nothing to finish
> +	 * it off, so deactivate it now.
> +	 */
> +	if (waiting)
> +		j1939_session_deactivate_activate_next(session);
>  }
>  
>  static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
> @@ -1279,10 +1317,8 @@ static enum hrtimer_restart j1939_tp_rxtimer(struct hrtimer *hrtimer)
>  		    session->state < J1939_SESSION_ACTIVE_MAX) {
>  			netdev_alert(priv->ndev, "%s: 0x%p: rx timeout, send abort\n",
>  				     __func__, session);
> -			j1939_session_get(session);
> -			hrtimer_start(&session->rxtimer,
> -				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
> -				      HRTIMER_MODE_REL_SOFT);
> +			j1939_tp_arm_rxtimer_locked(session,
> +						    J1939_XTP_ABORT_TIMEOUT_MS);
>  			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
>  		}
>  		j1939_session_list_unlock(session->priv);

[Severity: Medium]
Can j1939_tp_rxtimer() race with j1939_session_cancel() on session->state now
that j1939_session_cancel() no longer calls hrtimer_cancel(&session->rxtimer)
before setting J1939_SESSION_WAITING_ABORT?

CPU0                              CPU1
j1939_tp_rxtimer()
  if (session->state ==
      J1939_SESSION_WAITING_ABORT)
    -> false
                                  j1939_session_cancel()
                                    j1939_session_list_lock()
                                    __j1939_session_cancel()
                                      session->state =
                                        J1939_SESSION_WAITING_ABORT
                                    j1939_tp_arm_rxtimer_locked()
                                    j1939_session_list_unlock()
  j1939_session_list_lock()
  if (session->state >= J1939_SESSION_ACTIVE &&
      session->state < J1939_SESSION_ACTIVE_MAX)
    __j1939_session_cancel()  <-- cancels already-aborted session again

Because J1939_SESSION_WAITING_ABORT is less than J1939_SESSION_ACTIVE_MAX,
CPU0 sends a second abort, overwrites session->err with
J1939_XTP_ABORT_TIMEOUT, and rearms session->rxtimer while the callback is
running.  If CPU1 instead finishes before CPU0's unlocked check at the top of
j1939_tp_rxtimer(), CPU0 sees J1939_SESSION_WAITING_ABORT on the old timer's
expiration and immediately deactivates the session while the new abort timer
remains queued.

> @@ -1729,16 +1765,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;
>  	}

[Severity: Medium]
Does deactivating the session in j1939_xtp_rx_rts_session_active() and
j1939_session_cancel() leave session->rxtimer armed after the session is
removed from priv->active_session_list?

When session->last_cmd != 0 on an active transmitter session,
j1939_session_cancel() transitions the session to J1939_SESSION_WAITING_ABORT
and arms session->rxtimer for J1939_XTP_ABORT_TIMEOUT_MS, and
j1939_session_deactivate_activate_next() then immediately sets session->state
to J1939_SESSION_DONE and removes it from priv->active_session_list without
cancelling that timer.  The same happens when j1939_session_cancel() sees
waiting == true via callers such as j1939_tp_txtimer() that do not cancel
session->rxtimer beforehand, or when another CPU arms session->rxtimer between
j1939_session_timers_cancel() and j1939_session_list_lock().  Because the
deactivated session is no longer on priv->active_session_list,
j1939_cancel_active_session() cannot cancel the timer during socket close or
netdev teardown, and when the timer expires on an RX session (!session->sk) in
J1939_SESSION_DONE it falls into the else branch of j1939_tp_rxtimer() and
emits a duplicate J1939_ERRQUEUE_RX_ABORT notification.

Also, the commit message does not mention these state-machine changes in
j1939_session_cancel() and j1939_xtp_rx_rts_session_active(), and says
j1939_session_cancel() previously hand-rolled get + hrtimer_start() when it
actually called j1939_tp_set_rxtimeout().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-j1939-edpo-v9-v9-1-e779ca334a81@163.com?part=1

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

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Validate the packet number in ETP.CM_DPO, which is peer controlled and
currently accepted without any check.  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 a packet number 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, while 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.

Cancel the session timers before aborting, as the other receive-path abort
handlers do.  j1939_session_cancel() only rearms the abort timer and
leaves session->txtimer armed, so on the transmitter path the retry timer
keeps firing on an aborting session, retries up to J1939_XTP_TX_RETRY_LIMIT
and overwrites session->err with -ENETUNREACH; on the receiver path the
retries transmit CTS or EOMA and clobber the abort timeout.

The CTS packet number that feeds the transmitter-side EDPO is bounded in
4/4, on its own, because it is a different input parsed in a different
function; this patch only stops an EDPO value from being used as an
offset.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index fa23260e1..258009182 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1853,14 +1853,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 the responder controls, and 4/4 bounds it where it is parsed.
+	 */
+	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);
 

-- 
2.43.0

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

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Check the skb that j1939_session_completed() hands to j1939_sk_recv(),
which dereferences oskb->sk.  A NULL here is the last step of the EDPO
out-of-range sequence fixed by the previous patch; the bounds check makes
the lookup succeed, but the dereference should not depend on that.

Drop the message instead, which is what the other caller of
j1939_session_skb_get(), j1939_simple_txnext(), already does.

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

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 258009182..fc2840f33 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1281,9 +1281,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);

-- 
2.43.0

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

[Chaosheng Qu]

From: Chaosheng Qu <quchaosheng000406@163.com>

Reject a CTS packet number above pkt.total in j1939_xtp_rx_cts_one().
The function 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".

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Assisted-by: LLM
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 fc2840f33..377991d36 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1512,6 +1512,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 v9 4/4] can: j1939: reject a CTS that names more packets than the message has

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: j1939_xtp_rx_cts_one() rejects standard CTS(0)
  frames
--

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

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

This commit aborts a J1939 session with J1939_XTP_ABORT_ECTS_TOO_BIG in
j1939_xtp_rx_cts_one() when the received CTS packet number exceeds
session->pkt.total.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fc2840f334b5b..377991d36dc6a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1512,6 +1512,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;

[Severity: Medium]
Does this regression reject standard CTS(0) connection-hold frames?

Per SAE J1939-21 and ISO 11783-6, a CTS(0) frame (dat[1] == 0) sets the
next packet number field to 0xff for TP or 0xffffff for ETP.  Because this
check runs unconditionally even when dat[1] is 0, any session with
session->pkt.total smaller than 0xff (TP) or 0xffffff (ETP) will abort with
J1939_XTP_ABORT_ECTS_TOO_BIG upon receiving a CTS(0) instead of reaching
the CTS(0) timeout handler further down in j1939_xtp_rx_cts_one():

net/can/j1939/transport.c:j1939_xtp_rx_cts_one() {
	...
	if (dat[1]) {
		...
	} else {
		/* CTS(0) */
		j1939_tp_set_rxtimeout(session, 550);
	}
}

Also note that updating session->pkt.tx_acked, session->pkt.last, and
session->pkt.tx right below the new check when dat[1] == 0 contradicts the
"set packet counters only when not CTS(0)" comment unless both the new
bounds check and the packet counter updates are gated on dat[1] != 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-j1939-edpo-v9-v9-1-e779ca334a81@163.com?part=4