[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

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: j1939: stranded session in j1939_xtp_rx_dat_one() after bad EDPO
- [Medium] can: j1939: aborted ETP session in j1939_xtp_rx_dpo_one() still accepts data
--

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

can: j1939: validate the EDPO packet number

This commit validates the packet number in ETP.CM_DPO (EDPO) against
session->pkt.total and aborts the session if the offset is out of range.
It also makes j1939_session_cancel() return whether the abort timer was
armed and adds a NULL check in 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);
>  		__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;
>  }

[ ... ]

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

[Severity: High]
Can an ETP.DT frame arriving right after a rejected ETP.CM_DPO strand the
struct j1939_session and leak its net_device references?

When the first out-of-range ETP.CM_DPO arrives on an active receiving ETP
session from a non-local peer, j1939_xtp_rx_dpo_one() calls
j1939_session_cancel(), which transitions session->state to
J1939_SESSION_WAITING_ABORT, arms session->rxtimer for 500 ms, and returns
true. The session remains on priv->active_session_list with session->last_cmd
and session->pkt.dpo unchanged (and for a non-local peer SA, the echoed abort
frame has J1939_ECU_LOCAL_SRC set and J1939_ECU_LOCAL_DST unset, so
j1939_tp_cmd_recv() does not deactivate the receiving session on loopback).

In ETP, ETP.CM_DPO is immediately followed by ETP.DT data frames (such as in
the 5-frame sequence in the commit description). When that ETP.DT frame
arrives within 500 ms and fails validation in j1939_xtp_rx_dat_one() (for
example, because session->last_cmd was not updated to J1939_ETP_CMD_DPO, or
because dat[0] == 0 with session->pkt.dpo == 0 evaluates packet to -1), it
jumps to out_session_cancel:

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

Here j1939_session_timers_cancel() cancels the pending 500 ms abort rxtimer,
and j1939_session_cancel() sees session->state == J1939_SESSION_WAITING_ABORT,
does not re-arm rxtimer, and returns false. Because j1939_xtp_rx_dat_one()
(and similarly out_session_cancel in j1939_xtp_rx_cts_one()) ignores the
return value of j1939_session_cancel(), the session is left on
priv->active_session_list with no active timers, leaking struct j1939_session,
struct j1939_priv, and the net_device references if the socket is closed
before interface teardown.

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

[Severity: Medium]
What happens if a remote peer follows an out-of-range ETP.CM_DPO with a valid
in-range ETP.CM_DPO and ETP.DT frames before the 500 ms abort timeout expires?

After the first out-of-range ETP.CM_DPO puts the receiving session into
J1939_SESSION_WAITING_ABORT, the session stays on priv->active_session_list
for 500 ms with its previous session->pkt.dpo and session->last_cmd intact.

Because neither j1939_xtp_rx_dpo_one(), j1939_xtp_rx_dat_one(), nor
j1939_session_completed() checks session->state or session->err:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	switch (session->last_cmd) {
	case 0xff:
		break;
	case J1939_ETP_CMD_DPO:
		if (skcb->addr.type == J1939_ETP)
			break;
    ...
}

Wouldn't subsequent valid ETP.CM_DPO and ETP.DT frames received within that
500 ms window overwrite rxtimer, reassemble the payload, send ETP.CM_EOMA, and
deliver the already-aborted session's payload to user space via
j1939_session_completed() -> j1939_sk_recv()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020025036.74454-1-quchaosheng000406@163.com?part=1

[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