[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