[PATCH net v5] can: j1939: validate the EDPO packet number

[PATCH net v5] can: j1939: validate the EDPO packet number

[Chaosheng Qu]

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 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,
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".  That offset is past the end of the queued skb, so the
lookup returns NULL.  Before this patch the NULL was passed to
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 and abort the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET, the SAE J1939-21 abort code
for this case.  The check has to be ">=" and not ">": pkt.total is a
packet count, so the last valid packet is pkt.total - 1.

The EDPO is also looped back to the transmitter session.  A peer can make
that role see an out-of-range EDPO too by putting an oversized packet
number in ETP.CM_CTS: j1939_xtp_rx_cts_one() only rejects packet 0, and
stores "packet - 1" in pkt.tx_acked without comparing it to pkt.total.
j1939_session_tx_dpo() then copies pkt.tx_acked into the EDPO.  Rejecting
the looped-back EDPO aborts the transmission instead of letting it run
with an offset that cannot be encoded.  Validating ETP.CM_CTS is a
separate change.

The abort branch must not strand a session that is already waiting for
an abort.  j1939_session_cancel() re-arms the abort timer only while the
state is below J1939_SESSION_WAITING_ABORT.  On a second out-of-range
EDPO the first call has already moved the session to that state, so
j1939_session_timers_cancel() removes the pending timer and the second
j1939_session_cancel() does nothing.  Make j1939_session_cancel() report
whether it started the abort, and force-deactivate the receiver when it
did not.  This is the same recovery used by the second-RTS path.

Guard j1939_session_completed() against a NULL skb as defense in depth,
so a future regression in the offset arithmetic degrades to a dropped
message instead of a kernel panic.

Tested on v7.3.0-rc5 under QEMU, with the reproducer below:

  - an unpatched kernel panics in j1939_sk_recv()
  - the guard alone does not fix the bug: it drops the message but the
    transfer still completes, so it is not the fix for the bad offset
  - the range check alone survives the single-EDPO reproducer
  - two out-of-range EDPOs followed by two RTS frames leave the receiving
    session deactivated; no netdev reference remains stuck
  - a full 1786 byte ETP transfer between two J1939 sockets still
    completes with the payload intact.  Its last EDPO announces
    pkt.total - 1, one below the rejected boundary, and exercises both
    the receiver and the transmitter loopback

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>
---
v5:
 - Make j1939_session_cancel() report whether it armed the abort timer.
   A second out-of-range EDPO now force-deactivates a receiver that is
   already waiting for an abort instead of leaving it without a timer.
 - Drop the claim that the range check and the NULL guard are both
   required for the original reproducer.  Both survive that sequence;
   the guard is defense in depth, while the range check is the fix for
   the bad offset.
 - Drop the incorrect "one full packet past the end" description.  The
   offset starts six bytes past a 1786 byte reassembly skb.
 - Correct the transmitter-role statement.  An oversized ETP.CM_CTS can
   make the looped-back EDPO out of range, because the CTS path does not
   validate its packet number against pkt.total.
 - Add the Fixes tag.
 net/can/j1939/transport.c | 46 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..8e171b713859 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1158,13 +1158,16 @@ static void __j1939_session_cancel(struct j1939_session *session,
 		j1939_sk_send_loop_abort(session->sk, session->err);
 }
 
-static void j1939_session_cancel(struct j1939_session *session,
+static bool j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
+	bool active = false;
+
 	j1939_session_list_lock(session->priv);
 
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_WAITING_ABORT) {
+		active = true;
 		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		__j1939_session_cancel(session, err);
 	}
@@ -1173,6 +1176,8 @@ static void j1939_session_cancel(struct j1939_session *session,
 
 	if (!session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+
+	return active;
 }
 
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
@@ -1243,9 +1248,18 @@ 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);
+
+		/* The offset "pkt.dpo * 7" is not necessarily covered by a
+		 * queued skb, e.g. when a peer names a packet past the end of
+		 * the message in an EDPO.  j1939_simple_txnext() already
+		 * guards its call the same way; dropping the message is
+		 * better than handing a NULL skb to j1939_sk_recv().
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
@@ -1823,14 +1837,36 @@ 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.  It names the packet the announced window
+	 * starts at, and j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
+	 * value past the end moves pkt.dpo beyond the reassembled buffer and
+	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
+	 * no longer covered by any queued skb.
+	 */
+	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);
+		if (!j1939_session_cancel(session,
+					  J1939_XTP_ABORT_BAD_EDPO_OFFSET))
+			j1939_session_deactivate_activate_next(session);
+		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

Re: [PATCH net v5] can: j1939: validate the EDPO packet number

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 1 (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).

- [High] The patch adds a return value to j1939_session_cancel() so
  callers can tell when it did nothing because the session was already…
- [Medium] Parts of the commit message's account of the transmitter-role
  check are wrong, and the new branch sends a misleading abort code.
- [Low] The new comment in j1939_session_completed()
  (transport.c:1251-1256) gives 'when a peer names a packet past the end…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the helper the
  patch modifies has a deadlock.
- [Low] j1939_xtp_rx_dpo_one() now range-checks the EDPO value before
  storing it in pkt.dpo.

--

can: j1939: validate the EDPO packet number

This patch rejects an ETP.CM_DPO whose packet number is at or past
pkt.total and aborts the session with J1939_XTP_ABORT_BAD_EDPO_OFFSET.
It changes j1939_session_cancel() to report whether it started the abort,
so the new EDPO path can force-deactivate a session that is already
waiting for an abort. It also adds a NULL skb check to
j1939_session_completed().

Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..8e171b7138594 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1158,13 +1158,16 @@ static void __j1939_session_cancel(struct j1939_session *session,
>  		j1939_sk_send_loop_abort(session->sk, session->err);
>  }
>  
> -static void j1939_session_cancel(struct j1939_session *session,
> +static bool j1939_session_cancel(struct j1939_session *session,
>  				 enum j1939_xtp_abort err)
>  {
> +	bool active = false;
> +
>  	j1939_session_list_lock(session->priv);
>  
>  	if (session->state >= J1939_SESSION_ACTIVE &&
>  	    session->state < J1939_SESSION_WAITING_ABORT) {
> +		active = true;
>  		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);

[Severity: High]
This isn't a bug introduced by this patch, but can j1939_session_cancel()
deadlock against the rxtimer callback here?

The lock is held with spin_lock_bh() while j1939_tp_set_rxtimeout() does
a synchronous cancel:

j1939_session_cancel()
  j1939_session_list_lock()
  j1939_tp_set_rxtimeout()
    j1939_session_rxtimer_cancel()
      hrtimer_cancel(&session->rxtimer)

j1939_tp_rxtimer() takes the same lock on every branch. It takes it
directly in the active branch:

	} else {
		j1939_session_list_lock(session->priv);

and through j1939_session_deactivate() in the WAITING_ABORT and
J1939_SIMPLE branches.

Suppose CPU A is in j1939_tp_txtimer() and reaches the -EOVERFLOW case, or
the default case (for example -ENOENT or -ENOMEM). It calls
j1939_session_cancel() without cancelling the rxtimer first.

If CPU B is running this session's j1939_tp_rxtimer() at the same time and
is spinning on active_session_list_lock, then CPU A waits in
hrtimer_cancel() for CPU B while holding the lock CPU B needs.

The new EDPO caller calls j1939_session_timers_cancel() first, but the
txtimer callers don't. Could the helper cancel the rxtimer before taking
the lock, or re-arm it without a synchronous cancel?

[ ... ]

> @@ -1243,9 +1248,18 @@ 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);
> +
> +		/* The offset "pkt.dpo * 7" is not necessarily covered by a
> +		 * queued skb, e.g. when a peer names a packet past the end of
> +		 * the message in an EDPO.  j1939_simple_txnext() already
> +		 * guards its call the same way; dropping the message is
> +		 * better than handing a NULL skb to j1939_sk_recv().
> +		 */

[Severity: Low]
Can the example in this comment still happen after this patch?

The new check in j1939_xtp_rx_dpo_one() returns before pkt.dpo is assigned
whenever dpo >= pkt.total. For a receiver session pkt.dpo is therefore
either 0 or a validated value, and (pkt.total - 1) * 7 lies inside the
reassembly skb.

Should the comment describe the NULL check as defense in depth, as the
commit message does?

[ ... ]

> @@ -1823,14 +1837,36 @@ 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.  It names the packet the announced window
> +	 * starts at, and j1939_xtp_rx_dat_one() places data at
> +	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
> +	 * value past the end moves pkt.dpo beyond the reassembled buffer and
> +	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
> +	 * no longer covered by any queued skb.
> +	 */

[Severity: Low]
This is a pre-existing issue, but the comment says pkt.dpo "has to stay
inside the message". Only this writer enforces that. The other writer,
j1939_session_tx_dpo(), still does:

	session->pkt.dpo = session->pkt.tx_acked;

and tx_acked comes from an unchecked CTS packet number in
j1939_xtp_rx_cts_one():

	session->pkt.tx_acked = pkt - 1;

j1939_xtp_txnext_transmiter() then calls j1939_session_tx_dat() with that
value before the echo reaches the new check. In that case
j1939_session_skb_get_by_offset() returns NULL, so it fails safely with
-ENOBUFS retries and then -ENETUNREACH. The commit message also says CTS
validation will be a separate change.

Could the comment say that the transmitter-side writer is not covered
yet?

> +	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);
> +		if (!j1939_session_cancel(session,
> +					  J1939_XTP_ABORT_BAD_EDPO_OFFSET))
> +			j1939_session_deactivate_activate_next(session);

[Severity: High]
This handles a second out-of-range EDPO. What happens if the next bad
frame is an ETP.DT or an ETP.CM_CTS instead?

The other callers still ignore the new return value. In
j1939_xtp_rx_dat_one():

 out_session_cancel:
	kfree_skb(se_skb);
	j1939_session_timers_cancel(session);
	j1939_session_cancel(session, abort);
	j1939_session_put(session);

j1939_xtp_rx_cts_one() has the same out_session_cancel pattern, and so
does the first branch of j1939_xtp_rx_rts_session_active().

Take this sequence from a remote node:

  ETP.CM_RTS            -> receive session created
  ETP.CM_DPO (>= total) -> WAITING_ABORT, abort rxtimer armed
  ETP.DT with DLC != 8, or while last_cmd is still ETP_RTS
                        -> out_session_cancel in j1939_xtp_rx_dat_one()

j1939_session_get_by_addr() still finds the WAITING_ABORT session.
j1939_session_timers_cancel() removes the abort timer.
j1939_session_cancel() then does nothing, because the state is already
J1939_SESSION_WAITING_ABORT. Nothing is left to deactivate the session.

The receiver's own abort echo doesn't recover it. That echo is handled by
j1939_xtp_rx_abort(priv, skb, true), which only matches transmission
sessions.

Now suppose the last J1939 socket is closed. __j1939_rx_release() calls
j1939_priv_set(priv->ndev, NULL), so j1939_netdev_notify() can no longer
reach j1939_cancel_active_session().

Does that leave the session's netdev_hold() reference held forever, so
that unregister_netdevice() waits indefinitely?

The commit message says:

  The abort branch must not strand a session that is already waiting for
  an abort.

and:

  two out-of-range EDPOs followed by two RTS frames leave the receiving
  session deactivated; no netdev reference remains stuck

The timers_cancel() plus cancel() pattern predates this patch. Two
malformed DT frames can strand a session on their own. This patch adds
another peer-controlled way into WAITING_ABORT, though.

Would it be more robust to handle the already-WAITING_ABORT case inside
j1939_session_cancel(), or to stop callers from cancelling an armed abort
timer, instead of fixing each caller separately?

[Severity: Medium]
For a transmission session, this sends J1939_XTP_ABORT_BAD_EDPO_OFFSET to
the responder. A transmitter only sees an out-of-range EDPO here when the
responder sent an oversized packet number in ETP.CM_CTS. The in-tree enum
describes that case as:

	J1939_XTP_ABORT_ECTS_TOO_BIG = 15,
	/* ECTS requested packets exceeds message size */

j1939_tp_txtimer() already uses that code for -EOVERFLOW. Should the
transmitter role send J1939_XTP_ABORT_ECTS_TOO_BIG here, so the responder
isn't told its EDPO was bad when it never sent one?

A few statements in the commit message also don't seem to match the
code. It says:

  Rejecting the looped-back EDPO aborts the transmission instead of
  letting it run with an offset that cannot be encoded.

tx_acked is pkt - 1, where pkt is a non-zero 24-bit value, so
j1939_session_tx_dpo() writes it to dat[2..4] without truncation. Only
dat[1] = pkt.last - pkt.tx_acked wraps.

This check also runs on the CAN echo, after j1939_session_tx_dpo() has
already put the EDPO on the bus. Before the patch the transmitter didn't
run with that offset either. j1939_session_tx_dat() gets -ENOBUFS from the
skb lookup, and the session eventually ends with -ENETUNREACH.

It also says:

  Make j1939_session_cancel() report whether it started the abort, and
  force-deactivate the receiver when it did not.  This is the same
  recovery used by the second-RTS path.

The code force-deactivates both roles, and only when
j1939_session_cancel() returns false. The second-RTS path in
j1939_xtp_rx_rts_session_active() always deactivates the transmitter. It
deactivates the receiver whenever the state is J1939_SESSION_WAITING_ABORT
after the call.

Could the commit message be updated to match the code?

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

Re: [PATCH net v5] can: j1939: validate the EDPO packet number

[Quchaosheng]

v6 addresses both High findings, the Medium, and the two Low comments.

> [High] can j1939_session_cancel() deadlock against the rxtimer callback?

Yes, and j1939_tp_txtimer()'s -EOVERFLOW and default branches reach it
without cancelling the rxtimer first.  The fix is in the helper:

  j1939_session_cancel()
    j1939_session_list_lock()
    j1939_tp_set_rxtimeout()
      j1939_session_rxtimer_cancel()
        hrtimer_cancel()      <- waits for a callback that wants our lock

v6 cancels the rxtimer before taking the lock, then arms the abort timer
with hrtimer_start() under the lock.  hrtimer_start() does not wait for the
callback, so the cancel/arm ordering the callers rely on is preserved
without the synchronous cancel inside the critical section.

> [High] What happens if the next bad frame is an ETP.DT or an ETP.CM_CTS?

The session is stranded.  The timers are cancelled, j1939_session_cancel()
sees WAITING_ABORT and does nothing, and nothing is left to deactivate it.

v6 handles that in the helper: a session already in WAITING_ABORT is
deactivated there instead of being left for a timer the caller has just
cancelled.  That covers ETP.DT, ETP.CM_CTS and second-RTS without touching
any of them.  In j1939_xtp_rx_rts_session_active() the receiver branch then
does what the transmitter branch already did, so both become one
unconditional deactivation.

Measured on v7.3-rc7 under QEMU/KVM on vcan, one tree and one config
(defconfig + CONFIG_CAN_J1939=y, CONFIG_CAN_VCAN=y, CONFIG_LOCKDEP=y,
CONFIG_DEBUG_SPINLOCK=y), three kernels differing only in this hunk.  Each
case is a standalone guest.

             A           B             C           D
  stock      SLOT-STUCK  REF-DROPPED*  SLOT-STUCK  never returns
  v5         SLOT-STUCK  never returns SLOT-STUCK  never returns
  v6         SLOT-FREE   returns       SLOT-FREE   returns

  A  out-of-range EDPO, then an unparsable DT; is a second ETP.CM_RTS from
     the same SA/DA answered with a CTS?
  B  same as A, last socket closed, does `ip link del vcan0` return?
  C  two unparsable DTs, no EDPO; second RTS answered?
  D  two unparsable DTs; does `ip link del vcan0` return?

* B 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.
  A is still stuck on stock.

C and D are the case you described.  On stock the second unparsable DT
cancels the abort timer the first one armed, so the session stays on
active_session_list; D prints "unregister_netdevice: waiting for vcan0 to
become free. Usage count = 7" until the guest is killed.  Pre-existing, as
you said.  v5 fixed only the caller it added, so A through D are unchanged
from stock.

> [Medium] The transmitter role sends BAD_EDPO_OFFSET, and the commit
> message does not match the code

Both correct.  v6 sends J1939_XTP_ABORT_ECTS_TOO_BIG for a transmission
session and J1939_XTP_ABORT_BAD_EDPO_OFFSET for a receiver.  With a real
J1939 sender and an injected oversized CTS, v5 puts 12 on the wire and v6
puts 15.

The commit message now says what the code does:

  - tx_acked is pkt - 1 and is not truncated when written to dat[2..4];
    only dat[1] wraps.  "An offset that cannot be encoded" was wrong.
  - the check runs on the echo, after j1939_session_tx_dpo() has put the
    EDPO on the bus.  Before the patch the transmitter did not run with
    that offset either: j1939_session_tx_dat() failed with -ENOBUFS and the
    session ended in -ENETUNREACH.
  - the force-deactivation paragraph now describes the helper.

> [Low] The j1939_session_completed() comment

The example cannot happen with the range check in place, so the comment now
describes the guard as defense in depth.

> [Low] The comment says pkt.dpo "has to stay inside the message", but
> j1939_session_tx_dpo() has another, unbounded writer

The comment now says it bounds the receiver-side writer, names
j1939_session_tx_dpo() and the unchecked ETP.CM_CTS packet number, and
says that is a separate change.

The single-EDPO case and a legal 1786-byte transfer were run on all three
kernels as well: stock panics with "BUG: kernel NULL pointer dereference,
address: 0x18" in j1939_sk_recv(), and v6 delivers all 256 packets intact.

Not reproduced: the lockdep race the rxtimer change fixes.  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.

pw-bot: cr

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

Re: [PATCH net v5] can: j1939: validate the EDPO packet number

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5433 bytes --]

On 20.10.2026 10:50:36, Chaosheng Qu wrote:
> 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 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,
> 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".  That offset is past the end of the queued skb, so the
> lookup returns NULL.  Before this patch the NULL was passed to
> 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 and abort the
> session with J1939_XTP_ABORT_BAD_EDPO_OFFSET, the SAE J1939-21 abort code
> for this case.  The check has to be ">=" and not ">": pkt.total is a
> packet count, so the last valid packet is pkt.total - 1.
>
> The EDPO is also looped back to the transmitter session.  A peer can make
> that role see an out-of-range EDPO too by putting an oversized packet
> number in ETP.CM_CTS: j1939_xtp_rx_cts_one() only rejects packet 0, and
> stores "packet - 1" in pkt.tx_acked without comparing it to pkt.total.
> j1939_session_tx_dpo() then copies pkt.tx_acked into the EDPO.  Rejecting
> the looped-back EDPO aborts the transmission instead of letting it run
> with an offset that cannot be encoded.  Validating ETP.CM_CTS is a
> separate change.
>
> The abort branch must not strand a session that is already waiting for
> an abort.  j1939_session_cancel() re-arms the abort timer only while the
> state is below J1939_SESSION_WAITING_ABORT.  On a second out-of-range
> EDPO the first call has already moved the session to that state, so
> j1939_session_timers_cancel() removes the pending timer and the second
> j1939_session_cancel() does nothing.  Make j1939_session_cancel() report
> whether it started the abort, and force-deactivate the receiver when it
> did not.  This is the same recovery used by the second-RTS path.
>
> Guard j1939_session_completed() against a NULL skb as defense in depth,
> so a future regression in the offset arithmetic degrades to a dropped
> message instead of a kernel panic.
>
> Tested on v7.3.0-rc5 under QEMU, with the reproducer below:
>
>   - an unpatched kernel panics in j1939_sk_recv()
>   - the guard alone does not fix the bug: it drops the message but the
>     transfer still completes, so it is not the fix for the bad offset
>   - the range check alone survives the single-EDPO reproducer
>   - two out-of-range EDPOs followed by two RTS frames leave the receiving
>     session deactivated; no netdev reference remains stuck
>   - a full 1786 byte ETP transfer between two J1939 sockets still
>     completes with the payload intact.  Its last EDPO announces
>     pkt.total - 1, one below the rejected boundary, and exercises both
>     the receiver and the transmitter loopback
>
> 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>
> ---
> v5:
>  - Make j1939_session_cancel() report whether it armed the abort timer.
>    A second out-of-range EDPO now force-deactivates a receiver that is
>    already waiting for an abort instead of leaving it without a timer.
>  - Drop the claim that the range check and the NULL guard are both
>    required for the original reproducer.  Both survive that sequence;
>    the guard is defense in depth, while the range check is the fix for
>    the bad offset.
>  - Drop the incorrect "one full packet past the end" description.  The
>    offset starts six bytes past a 1786 byte reassembly skb.
>  - Correct the transmitter-role statement.  An oversized ETP.CM_CTS can
>    make the looped-back EDPO out of range, because the CTS path does not
>    validate its packet number against pkt.total.
>  - Add the Fixes tag.

Thanks for sticking with it and continuing to work on the patch. A few
remarks regarding the process:

- only send the series/patch once

Some comments regarding the change log:

- keep it short and dense
- bullet points are preferred over prose text
- only add description of changes compared to the last version
- the rationale for those changes are not that important, as the
  motivation for the whole patch should be explained in the patch
  description

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net v5] can: j1939: validate the EDPO packet number

[Chaosheng Qu]

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 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,
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".  That offset is past the end of the queued skb, so the
lookup returns NULL.  Before this patch the NULL was passed to
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 and abort the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET, the SAE J1939-21 abort code
for this case.  The check has to be ">=" and not ">": pkt.total is a
packet count, so the last valid packet is pkt.total - 1.

The EDPO is also looped back to the transmitter session.  A peer can make
that role see an out-of-range EDPO too by putting an oversized packet
number in ETP.CM_CTS: j1939_xtp_rx_cts_one() only rejects packet 0, and
stores "packet - 1" in pkt.tx_acked without comparing it to pkt.total.
j1939_session_tx_dpo() then copies pkt.tx_acked into the EDPO.  Rejecting
the looped-back EDPO aborts the transmission instead of letting it run
with an offset that cannot be encoded.  Validating ETP.CM_CTS is a
separate change.

The abort branch must not strand a session that is already waiting for
an abort.  j1939_session_cancel() re-arms the abort timer only while the
state is below J1939_SESSION_WAITING_ABORT.  On a second out-of-range
EDPO the first call has already moved the session to that state, so
j1939_session_timers_cancel() removes the pending timer and the second
j1939_session_cancel() does nothing.  Make j1939_session_cancel() report
whether it started the abort, and force-deactivate the receiver when it
did not.  This is the same recovery used by the second-RTS path.

Guard j1939_session_completed() against a NULL skb as defense in depth,
so a future regression in the offset arithmetic degrades to a dropped
message instead of a kernel panic.

Tested on v7.3.0-rc5 under QEMU, with the reproducer below:

  - an unpatched kernel panics in j1939_sk_recv()
  - the guard alone does not fix the bug: it drops the message but the
    transfer still completes, so it is not the fix for the bad offset
  - the range check alone survives the single-EDPO reproducer
  - two out-of-range EDPOs followed by two RTS frames leave the receiving
    session deactivated; no netdev reference remains stuck
  - a full 1786 byte ETP transfer between two J1939 sockets still
    completes with the payload intact.  Its last EDPO announces
    pkt.total - 1, one below the rejected boundary, and exercises both
    the receiver and the transmitter loopback

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>
---
v5:
 - Make j1939_session_cancel() report whether it armed the abort timer.
   A second out-of-range EDPO now force-deactivates a receiver that is
   already waiting for an abort instead of leaving it without a timer.
 - Drop the claim that the range check and the NULL guard are both
   required for the original reproducer.  Both survive that sequence;
   the guard is defense in depth, while the range check is the fix for
   the bad offset.
 - Drop the incorrect "one full packet past the end" description.  The
   offset starts six bytes past a 1786 byte reassembly skb.
 - Correct the transmitter-role statement.  An oversized ETP.CM_CTS can
   make the looped-back EDPO out of range, because the CTS path does not
   validate its packet number against pkt.total.
 - Add the Fixes tag.
 net/can/j1939/transport.c | 46 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..8e171b713859 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1158,13 +1158,16 @@ static void __j1939_session_cancel(struct j1939_session *session,
 		j1939_sk_send_loop_abort(session->sk, session->err);
 }
 
-static void j1939_session_cancel(struct j1939_session *session,
+static bool j1939_session_cancel(struct j1939_session *session,
 				 enum j1939_xtp_abort err)
 {
+	bool active = false;
+
 	j1939_session_list_lock(session->priv);
 
 	if (session->state >= J1939_SESSION_ACTIVE &&
 	    session->state < J1939_SESSION_WAITING_ABORT) {
+		active = true;
 		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
 		__j1939_session_cancel(session, err);
 	}
@@ -1173,6 +1176,8 @@ static void j1939_session_cancel(struct j1939_session *session,
 
 	if (!session->sk)
 		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
+
+	return active;
 }
 
 static enum hrtimer_restart j1939_tp_txtimer(struct hrtimer *hrtimer)
@@ -1243,9 +1248,18 @@ 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);
+
+		/* The offset "pkt.dpo * 7" is not necessarily covered by a
+		 * queued skb, e.g. when a peer names a packet past the end of
+		 * the message in an EDPO.  j1939_simple_txnext() already
+		 * guards its call the same way; dropping the message is
+		 * better than handing a NULL skb to j1939_sk_recv().
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
@@ -1823,14 +1837,36 @@ 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.  It names the packet the announced window
+	 * starts at, and j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
+	 * value past the end moves pkt.dpo beyond the reassembled buffer and
+	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
+	 * no longer covered by any queued skb.
+	 */
+	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);
+		if (!j1939_session_cancel(session,
+					  J1939_XTP_ABORT_BAD_EDPO_OFFSET))
+			j1939_session_deactivate_activate_next(session);
+		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