can: j1939: fix the abort timer and bound EDPO

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 <[email protected]>

From: Chaosheng Qu <[email protected]>

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.

---
 net/can/j1939/transport.c | 66 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 18 deletions(-)

index 8fcfd13e5..fa23260e1 100644
@@ -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

From: Chaosheng Qu <[email protected]>

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.

---
 net/can/j1939/transport.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

index fa23260e1..258009182 100644
@@ -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

From: Chaosheng Qu <[email protected]>

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.

---
 net/can/j1939/transport.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

index 258009182..fc2840f33 100644
@@ -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

From: Chaosheng Qu <[email protected]>

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

---
 net/can/j1939/transport.c | 9 +++++++++
 1 file changed, 9 insertions(+)

index fc2840f33..377991d36 100644
@@ -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