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

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

[Quchaosheng]

From: Chaosheng Qu <quchaosheng000406@163.com>

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

Reaching the dereference needs two things, and this patch keeps them as
two separate changes because neither is sufficient alone:

  - the range check in j1939_xtp_rx_dpo_one(), which stops pkt.dpo from
    leaving the message
  - the NULL guard in j1939_session_completed(), which covers the offset
    lookup coming up empty for any other reason

The reproducer in the link was run three times in each configuration: the
unpatched kernel panics 3/3, a kernel with only the NULL guard still
panics 3/3, and this patch gives 0/3.  The middle result is the point --
it shows the guard does not close this hole on its own, so the range check
is not redundant with it.

Nothing on the path checks a capability: net/can has no CAP_NET_RAW check
at all, CAN_J1939 does not require one, and the only capability gate
nearby is SO_J1939_SEND_PRIO < 2, which asks for CAP_NET_ADMIN and is not
needed here.  The vcan interface used by the reproducer has to be created
first, which normally needs privileges; given the interface, any user can
reach this.

  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

J1939_XTP_ABORT_BAD_EDPO_OFFSET and its "Bad EDPO offset" string are
already in the tree and have no caller; the range check uses that abort.

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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..065b4f71e9af 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1243,9 +1243,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 +1832,34 @@ 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);
+		j1939_session_cancel(session, 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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 3

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

- [Medium] The commit message's justification does not match the code.
- [Medium] The new out-of-range EDPO branch in j1939_xtp_rx_dpo_one()
  calls j1939_session_timers_cancel() and then j1939_session_cancel().
- [Low] The patch fixes a NULL pointer dereference that any CAN peer, or
  any local user with a CAN/vcan interface, can trigger.
- [Low] The new EDPO range check in j1939_xtp_rx_dpo_one() assumes the
  EDPO always comes from the remote peer (its comment says "EDPO is peer…
- [Low] The new comment in j1939_session_completed() explains the NULL
  guard with an example: "e.g. when a peer names a packet past the end…

--

I couldn't run the reproducer. Everything below comes from reading the code at c9baa98874f5.

can: j1939: validate the EDPO packet number

This commit adds a range check on the ETP.CM_DPO packet number in
j1939_xtp_rx_dpo_one(). If the value is not below pkt.total, the session
is aborted with J1939_XTP_ABORT_BAD_EDPO_OFFSET. It also stops
j1939_session_completed() from passing a NULL skb from
j1939_session_skb_get() to j1939_sk_recv().

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

> Reaching the dereference needs two things, and this patch keeps them as
> two separate changes because neither is sufficient alone:

[ ... ]

> The reproducer in the link was run three times in each configuration: the
> unpatched kernel panics 3/3, a kernel with only the NULL guard still
> panics 3/3, and this patch gives 0/3.  The middle result is the point --
> it shows the guard does not close this hole on its own, so the range check
> is not redundant with it.

[Severity: Medium]
Is this description accurate? From the code, the NULL guard alone looks
like it should stop the dereference described here.

j1939_session_skb_get() has two callers. j1939_simple_txnext() already
checks for NULL, and j1939_session_completed() is now guarded.

j1939_sk_recv() also has two callers. j1939_can_recv() passes a real skb,
and the other caller is j1939_session_completed().

With the guard in place, how does a NULL se_skb still reach this
dereference?

net/can/j1939/socket.c:j1939_sk_recv_one() {
    ...
	if (oskb->sk == &jsk->sk)
		return;
    ...
}

The range check alone also seems to cover the described trigger. For an
ETP receive session, j1939_xtp_rx_rts_session_new() sets:

	session->pkt.total = (len + 6) / 7;

The session then has a single skb of length len at offset 0. So
dpo < pkt.total gives dpo * 7 < len, and
j1939_session_skb_get_by_offset() always finds that skb.
j1939_xtp_rx_dpo_one() is the only place a receive session's pkt.dpo is
written.

If the guard-only kernel really panicked 3/3, is there a second crash site
that the commit message doesn't name? If not, could the guard-only test
setup be checked again?

The message also says the patch "keeps them as two separate changes", but
both changes are in this one commit. Should they be split into two patches,
or should the wording change?

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

[Severity: Low]
This fixes a NULL pointer dereference in j1939_sk_recv_one(), reached
through j1939_session_completed(), and a CAN peer can trigger it. Should
it carry a Fixes: tag and Cc: stable?

The unvalidated assignment in j1939_xtp_rx_dpo_one() comes from
9d71dd0c7009 ("can: add support of SAE J1939 protocol"):

	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);

Blame on the unguarded j1939_sk_recv() call in j1939_session_completed()
points to 7ac56e40d054 ("can: j1939: j1939_session_completed(): use
consistent name se_skb for the session skb"). That commit only renamed the
variable, so 9d71dd0c7009 looks like the right Fixes: target.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..065b4f71e9af8 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1243,9 +1243,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 this example still happen? The same patch makes j1939_xtp_rx_dpo_one()
reject any dpo >= pkt.total before it writes session->pkt.dpo, and that is
the only place a receive session's pkt.dpo is written.

pkt.total is (len + 6) / 7 and the receive skb covers [0, len), so an EDPO
past the end of the message can no longer reach this guard.

Could the comment name a case that can still return NULL, or just say the
check is defensive?

> +		if (se_skb) {
> +			/* distribute among j1939 receivers */
> +			j1939_sk_recv(session->priv, se_skb);
> +			consume_skb(se_skb);
> +		}
>  	}

[ ... ]

> @@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *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);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);

[Severity: Low]
Does the EDPO checked here always come from the peer?
j1939_tp_cmd_recv() also sends the originator's own looped-back
ETP.CM_DPO to this function:

	if (j1939_tp_im_transmitter(skcb))
		j1939_xtp_rx_dpo(priv, skb, true);

On the originator, j1939_session_tx_dpo() builds the EDPO from:

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

j1939_xtp_rx_cts_one() sets tx_acked from the peer's ECTS:

	session->pkt.tx_acked = pkt - 1;

That packet number is only checked for being non-zero, never against
pkt.total. So a peer ECTS with a packet number greater than pkt.total makes
the originator's own EDPO hit this check.

The transmit session is then aborted with J1939_XTP_ABORT_BAD_EDPO_OFFSET,
and the log says the EDPO is out of range.

Would J1939_XTP_ABORT_ECTS_TOO_BIG be the better abort code on the
transmitter side? The local errno is EPROTO either way, so only the code
sent on the bus and the log message differ.

> +		return;
> +	}

[Severity: Medium]
Can this leave a receive session that is already in
J1939_SESSION_WAITING_ABORT with no timer?

Say a peer sends two out-of-range EDPOs with the same PGN, the second one
within J1939_XTP_ABORT_TIMEOUT_MS of the first.

First EDPO:
  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()
    j1939_session_cancel()
      j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS)
      __j1939_session_cancel()   -> state = WAITING_ABORT, ABORT sent

Second EDPO:
  j1939_session_get_by_addr_locked()   -> still finds the session
  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()      -> cancels the abort rxtimer
    j1939_session_cancel()             -> does nothing, because of:

	if (session->state >= J1939_SESSION_ACTIVE &&
	    session->state < J1939_SESSION_WAITING_ABORT) {

Nothing re-arms rxtimer after that. So the WAITING_ABORT branch in
j1939_tp_rxtimer() never reaches j1939_session_deactivate_activate_next().

The echo of our own ABORT doesn't deactivate the session either. It
arrives with J1939_ECU_LOCAL_SRC, and j1939_xtp_rx_abort(priv, skb, true)
only matches transmission sessions.

The session then stays on priv->active_session_list. It holds its kref,
its priv and netdev references and the reassembly skb. That lasts until
the peer sends an ABORT, a new RTS or a valid DPO, or the netdev goes down.

Before this patch, every DPO went through
j1939_tp_set_rxtimeout(session, 750), which re-armed the timer.

Commit 1809c82aa073 ("net: can: j1939: j1939_xtp_rx_rts_session_active():
deactivate session upon receiving the second rts") fixed the same case for
the RTS path:

	} else if (session->state == J1939_SESSION_WAITING_ABORT) {
		/* Force deactivation for the receiver.
		...
		j1939_session_deactivate_activate_next(session);
	}

Should the DPO path do the same? Or should it skip
j1939_session_timers_cancel() when the session is already aborting?

j1939_xtp_rx_dat_one() and j1939_xtp_rx_cts_one() already use the same
cancel-then-cancel sequence. This patch adds one more frame type that can
trigger it.

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

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

[Quchaosheng]

From: Chaosheng Qu <quchaosheng000406@163.com>

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

Reaching the dereference needs two things, and this patch keeps them as
two separate changes because neither is sufficient alone:

  - the range check in j1939_xtp_rx_dpo_one(), which stops pkt.dpo from
    leaving the message
  - the NULL guard in j1939_session_completed(), which covers the offset
    lookup coming up empty for any other reason

The reproducer in the link was run three times in each configuration: the
unpatched kernel panics 3/3, a kernel with only the NULL guard still
panics 3/3, and this patch gives 0/3.  The middle result is the point --
it shows the guard does not close this hole on its own, so the range check
is not redundant with it.

Nothing on the path checks a capability: net/can has no CAP_NET_RAW check
at all, CAN_J1939 does not require one, and the only capability gate
nearby is SO_J1939_SEND_PRIO < 2, which asks for CAP_NET_ADMIN and is not
needed here.  The vcan interface used by the reproducer has to be created
first, which normally needs privileges; given the interface, any user can
reach this.

  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

J1939_XTP_ABORT_BAD_EDPO_OFFSET and its "Bad EDPO offset" string are
already in the tree and have no caller; the range check uses that abort.

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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..065b4f71e9af 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1243,9 +1243,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 +1832,34 @@ 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);
+		j1939_session_cancel(session, 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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 3

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 new range check in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c:1852-1858) can leave a receive session…
- [Medium] Part of the commit message's reasoning does not match the
  code.
- [Low] The patch fixes a NULL-pointer dereference that a remote CAN node
  can trigger through crafted ETP.CM_RTS/ETP.CM_DPO/ETP.DT frames, but…
- [Low] The new check in j1939_xtp_rx_dpo_one() also runs for transmit
  sessions, via the looped-back local DPO: j1939_tp_cmd_recv() calls…
- [Low] The new comment in j1939_session_completed() (lines 1247-1252)
  names 'when a peer names a packet past the end of the message in an…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  another caller that can reach it.

--

can: j1939: validate the EDPO packet number

This commit adds a range check on the ETP.CM_DPO packet number in
j1939_xtp_rx_dpo_one(). If the number is not below pkt.total, the session
is aborted with J1939_XTP_ABORT_BAD_EDPO_OFFSET. It also adds a NULL check
in j1939_session_completed() on the skb returned by
j1939_session_skb_get(), before that skb is passed to j1939_sk_recv().

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

> Reaching the dereference needs two things, and this patch keeps them as
> two separate changes because neither is sufficient alone:
>
>   - the range check in j1939_xtp_rx_dpo_one(), which stops pkt.dpo from
>     leaving the message
>   - the NULL guard in j1939_session_completed(), which covers the offset
>     lookup coming up empty for any other reason
>
> The reproducer in the link was run three times in each configuration: the
> unpatched kernel panics 3/3, a kernel with only the NULL guard still
> panics 3/3, and this patch gives 0/3.  The middle result is the point --
> it shows the guard does not close this hole on its own, so the range check
> is not redundant with it.

[Severity: Medium]
Is this description accurate? With only the NULL guard applied, the
reproducer sequence in the message seems to go like this:

  ETP.CM_DPO (packet 256) -> pkt.dpo = 256
  ETP.DT seq 0            -> packet 255, passes "packet > pkt.total"
  j1939_session_skb_get_by_offset(session, 1785) finds the 1786 byte skb
  pkt.rx reaches 256, the receiver sends EOMA

The loopback of that EOMA then runs:

  j1939_xtp_rx_eoma_one()
    j1939_session_completed()
      se_skb = j1939_session_skb_get(session);  /* offset 1792, NULL */
      if (se_skb) {                             /* j1939_sk_recv() skipped */

The other users of pkt.dpo on this path all handle a NULL lookup already:
j1939_simple_txnext(), j1939_session_tx_dat() and j1939_xtp_rx_dat_one().
j1939_sk_queue_activate_next() returns early for receive sessions with
sk == NULL.

The reverse case also seems to hold. With only the range check, dpo < total
gives dpo * 7 <= (total - 1) * 7 < total_message_size. The single reassembly
skb at offset 0 then always covers the lookup.

Where does the guard-only kernel panic in the 3/3 runs? Is there a second
crash site that the message doesn't describe?

Also, the message says the patch "keeps them as two separate changes", but
this is a single commit.

[ ... ]

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

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? A remote CAN node can trigger
the NULL dereference with crafted ETP.CM_RTS, ETP.CM_DPO and ETP.DT frames.

The unvalidated assignment in j1939_xtp_rx_dpo_one() and the outline of
j1939_session_completed() both come from:

  Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

The unguarded se_skb lookup line was last changed in 7ac56e40d054.

The message describes reachability in terms of a local vcan interface. On
a real bus, can't any remote node send this sequence to a bound J1939
receiver?

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..065b4f71e9af8 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1243,9 +1243,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]
This isn't a bug, but the comment gives an EDPO past the end of the message
as its example. The range check added to j1939_xtp_rx_dpo_one() by this same
patch now rejects that case.

For receive sessions, pkt.dpo is written only in j1939_xtp_rx_dpo_one() and
starts at 0. The single reassembly skb sits at offset 0 with length
total_message_size. Since (total - 1) * 7 < total_message_size, the lookup
should always succeed after this patch.

Could the comment instead describe what the guard still protects against?

> +		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 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

> +	if (dpo >= session->pkt.total) {

[Severity: Low]
This check also runs for transmit sessions, through the looped-back local
DPO. When LOCAL_SRC is set, j1939_tp_cmd_recv() calls
j1939_xtp_rx_dpo(priv, skb, true).

The transmit side can still produce a pkt.dpo past the end of the message.
j1939_xtp_rx_cts_one() only rejects pkt == 0, and it doesn't compare pkt
against pkt.total:

	if (!pkt)
		goto out_session_cancel;
	...
	session->pkt.tx_acked = pkt - 1;

j1939_session_tx_dpo() then sends that value:

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

When the DPO echoes back, the new check aborts the transmit session with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. That tells the peer our EDPO offset was
bad, but the actual fault was its oversized ETP.CM_CTS.

Would J1939_XTP_ABORT_ECTS_TOO_BIG, or a pkt.total check in
j1939_xtp_rx_cts_one(), be more accurate here? This doesn't crash, and the
local errno is EPROTO either way.

It also means the statement that the check "stops pkt.dpo from leaving the
message" only holds for receive sessions.

> +		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);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: High]
Can this leave a receive session stuck forever? Suppose a peer sends two
ETP.CM_DPO frames, both with an offset >= pkt.total, less than 500 ms apart.

The first bad DPO:

  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()
    j1939_session_cancel()
      j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS)
      __j1939_session_cancel()       /* state -> WAITING_ABORT */

j1939_session_get_by_addr_locked() doesn't filter on state, so the second
bad DPO still finds the session:

  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()    /* cancels the pending abort rxtimer */
    j1939_session_cancel()           /* WAITING_ABORT, nothing re-armed */

For a receive session, the only code that force-deactivates it is the
WAITING_ABORT branch of j1939_tp_rxtimer(). The looped-back abort only
matches sessions with transmission == true.

That leaves the session on active_session_list with no timer armed. It keeps
its kref, the reassembly skb, the priv reference and the netdev_hold()
reference from j1939_session_new().

If the last J1939 socket is then closed, __j1939_rx_release() clears priv
from the netdev. After that, j1939_netdev_notify() can't reach the session,
and unregister_netdevice would wait forever on the netdev reference.

Before this patch, a DPO in WAITING_ABORT went through
j1939_tp_set_rxtimeout(session, 750), which re-armed the timer, so this path
used to recover on its own. A similar stranding was already possible through
the j1939_xtp_rx_dat_one() error path.

j1939_xtp_rx_rts_session_active() already works around this exact problem:

		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);
		}

Should the DPO path either skip the cancel when the session is already in
WAITING_ABORT, or force deactivation the way the RTS path does?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but the
patch adds one more caller that can reach it.

j1939_session_cancel() holds the session list lock (spin_lock_bh) while it
calls j1939_tp_set_rxtimeout(). That calls j1939_session_rxtimer_cancel(),
which uses hrtimer_cancel() and waits for a running callback.
j1939_tp_rxtimer() takes the same lock, directly in its else branch and
through j1939_session_deactivate() in the other branches.

Can these two deadlock?

  CPU A                                CPU C
  j1939_session_cancel()
    j1939_session_list_lock()
                                       j1939_tp_rxtimer()
                                         j1939_session_list_lock()
                                         /* spins on the lock */
    j1939_tp_set_rxtimeout()
      j1939_session_rxtimer_cancel()
        hrtimer_cancel()
        /* waits for CPU C's callback */

In the new branch, j1939_session_timers_cancel() runs before
j1939_session_cancel(), but without the lock. Another context can re-arm the
rxtimer in between. Examples are a concurrent valid DPO or DAT frame, or a
txtimer-driven CTS or EOMA calling j1939_tp_set_rxtimeout().

j1939_tp_txtimer() also calls j1939_session_cancel() on -EOVERFLOW and in
its default case without cancelling the timers first.

Under the same lock, j1939_cancel_active_session() uses
hrtimer_try_to_cancel() instead:

			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
				j1939_session_put(session);

Would j1939_session_cancel() need a similar non-waiting cancel?

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

[ ... ]

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

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

[Quchaosheng]

From: Chaosheng Qu <quchaosheng000406@163.com>

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

Reaching the dereference needs two things, and this patch keeps them as
two separate changes because neither is sufficient alone:

  - the range check in j1939_xtp_rx_dpo_one(), which stops pkt.dpo from
    leaving the message
  - the NULL guard in j1939_session_completed(), which covers the offset
    lookup coming up empty for any other reason

The reproducer in the link was run three times in each configuration: the
unpatched kernel panics 3/3, a kernel with only the NULL guard still
panics 3/3, and this patch gives 0/3.  The middle result is the point --
it shows the guard does not close this hole on its own, so the range check
is not redundant with it.

Nothing on the path checks a capability: net/can has no CAP_NET_RAW check
at all, CAN_J1939 does not require one, and the only capability gate
nearby is SO_J1939_SEND_PRIO < 2, which asks for CAP_NET_ADMIN and is not
needed here.  The vcan interface used by the reproducer has to be created
first, which normally needs privileges; given the interface, any user can
reach this.

  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

J1939_XTP_ABORT_BAD_EDPO_OFFSET and its "Bad EDPO offset" string are
already in the tree and have no caller; the range check uses that abort.

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 | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..065b4f71e9af 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1243,9 +1243,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 +1832,34 @@ 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);
+		j1939_session_cancel(session, 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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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

- [Medium] The commit message's main reason for having two hunks does not
  match the code.
- [Medium] The new EDPO abort branch in j1939_xtp_rx_dpo_one() calls
  j1939_session_timers_cancel() and then j1939_session_cancel() without…
- [Low] The patch fixes a kernel NULL pointer dereference that a peer can
  trigger: a NULL se_skb reaches j1939_sk_recv() from…
- [Low] The new comment in j1939_session_completed() justifies the NULL
  guard with "when a peer names a packet past the end of the message in…

--

can: j1939: validate the EDPO packet number

This patch adds a range check on the ETP.CM_DPO packet number, which
comes from the peer, in j1939_xtp_rx_dpo_one(). If the number is not
below pkt.total, the session is aborted with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. The patch also adds a NULL check on the
skb returned by j1939_session_skb_get() in j1939_session_completed(),
before it is passed to j1939_sk_recv().

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

> Reaching the dereference needs two things, and this patch keeps them as
> two separate changes because neither is sufficient alone:

[ ... ]

> The reproducer in the link was run three times in each configuration: the
> unpatched kernel panics 3/3, a kernel with only the NULL guard still
> panics 3/3, and this patch gives 0/3.

[Severity: Medium]
Is this accurate? For the sequence in the commit message, it looks like
each hunk would be enough on its own.

With only the NULL guard, pkt.dpo == 256 and pkt.total == 256, and
j1939_session_completed() calls j1939_session_skb_get():

net/can/j1939/transport.c:j1939_session_skb_get() {
	offset_start = session->pkt.dpo * 7;
	return j1939_session_skb_get_by_offset(session, offset_start);
}

For offset 1792 this returns NULL, and the guard then skips
j1939_sk_recv().

The other users of pkt.dpo don't seem able to fault either.
j1939_simple_txnext() already returns 0 on a NULL skb.
j1939_xtp_rx_dat_one() rejects packet > total, and a failed
j1939_session_skb_get_by_offset() there goes to out_session_cancel.

With only the range check, dpo < total gives
dpo * 7 <= (total - 1) * 7 < total_message_size. So the lookup always
lands inside the single reassembly skb that j1939_session_fresh_new()
allocates.

If the guard-only kernel really panics 3/3, could that be a different
crash from the one described here? Or should the test result be checked
again?

A smaller point in the same message:

> j1939_session_skb_get() for offset "pkt.dpo * 7" -- one full packet past
> the end of the queued skb -- gets NULL

The message is 1786 bytes, so 256 * 7 = 1792 is 6 bytes past the end of
the skb, not one full packet past it.

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

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? Before this patch,
j1939_xtp_rx_dpo_one() stored j1939_etp_ctl_to_packet() into pkt.dpo
without any check. That leads to a NULL pointer dereference in
j1939_sk_recv(), reached from j1939_session_completed().

The commit message describes a local user on vcan. On a real CAN bus,
though, it looks like any remote node could send the crafted
ETP.CM_RTS, ETP.CM_DPO and ETP.DT frames. The only requirement seems to
be a local J1939 socket that matches the session, through
j1939_sk_recv_match() in j1939_xtp_rx_rts_session_new(). In that case no
local privileges are needed.

A Fixes: tag pointing at the commit that added the unchecked pkt.dpo
assignment would help stable triage.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..065b4f71e9af8 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1243,9 +1243,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]
This isn't a bug, but can the EDPO example in this comment still happen?
The range check this patch adds to j1939_xtp_rx_dpo_one() keeps pkt.dpo
below pkt.total for receive sessions. And (total - 1) * 7 always falls
inside the single reassembly skb.

Would it be clearer to describe the guard as defensive, or to name a
case that can still happen after this patch?

> +		if (se_skb) {
> +			/* distribute among j1939 receivers */
> +			j1939_sk_recv(session->priv, se_skb);
> +			consume_skb(se_skb);
> +		}
>  	}

[ ... ]

> @@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

> +	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);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: Medium]
Can a second out of range EDPO leave the session stuck on
priv->active_session_list?

The first bad EDPO goes through j1939_session_cancel(). That arms the
abort rxtimer and moves the session to J1939_SESSION_WAITING_ABORT.

j1939_session_get_by_addr_locked() doesn't filter by state. So a second
bad EDPO within J1939_XTP_ABORT_TIMEOUT_MS reaches this branch again:

j1939_xtp_rx_dpo_one()
  j1939_session_timers_cancel()       <- cancels the pending abort rxtimer
  j1939_session_cancel()
    if (session->state >= J1939_SESSION_ACTIVE &&
        session->state < J1939_SESSION_WAITING_ABORT) {   <- false
            j1939_tp_set_rxtimeout(...);   <- not re-armed

The only automatic path that deactivates a WAITING_ABORT session is
j1939_tp_rxtimer(). With the timer cancelled, the session would keep its
reassembly skb (up to tp_max_packet_size), the netdev tracker and the
j1939_priv reference. These would be held until the peer sends an abort
or a new RTS, or the interface is torn down.

Before this patch, a DPO in WAITING_ABORT went through
j1939_tp_set_rxtimeout(session, 750), which re-armed the timer.

j1939_xtp_rx_rts_session_active() already works around this same
problem:

	} 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);
	}

Would something similar be needed here? The same timers_cancel plus
session_cancel pattern at out_session_cancel in j1939_xtp_rx_dat_one()
seems to have the same weakness.

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