Linux CAN drivers development
 help / color / mirror / Atom feed
* [PATCH net v6 1/1] can: j1939: validate the EDPO packet number
@ 2026-10-22  7:42 Quchaosheng
  2026-10-22  8:03 ` sashiko-bot
  2026-10-23 20:51 ` netdev-bot+sashiko
  0 siblings, 2 replies; 4+ messages in thread
From: Quchaosheng @ 2026-10-22  7:42 UTC (permalink / raw)
  To: Robin van der Gracht, Oleksij Rempel, kernel
  Cc: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel,
	Sashiko


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

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

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

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

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

j1939_session_cancel() changes in two ways:

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

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

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

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

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

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

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

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

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

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

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

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

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-25  2:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22  7:42 [PATCH net v6 1/1] can: j1939: validate the EDPO packet number Quchaosheng
2026-10-22  8:03 ` sashiko-bot
2026-10-23 20:51 ` netdev-bot+sashiko
2026-10-25  2:18   ` Chaosheng Qu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox