Message-ID: <sashiko-outbox-208835@kernel.org>
From: sashiko-bot@kernel.org
Subject: Re: [PATCH net v9 4/4] can: j1939: reject a CTS that names more
 packets than the message has
Reply-To: sashiko-reviews@lists.linux.dev
To: "Chaosheng Qu" <chaosheng.qu.linux@gmail.com>
Cc: linux-can@vger.kernel.org, "Marc Kleine-Budde" <mkl@pengutronix.de>, "Oliver
 Hartkopp" <socketcan@hartkopp.net>, "Vincent Mailhol" <mailhol@kernel.org>, "Oleksij
 Rempel" <o.rempel@pengutronix.de>
In-Reply-To: <20261111-j1939-edpo-v9-v9-4-e779ca334a81@163.com>
References: <20261111-j1939-edpo-v9-v9-0-e779ca334a81@163.com>
 <20261111-j1939-edpo-v9-v9-4-e779ca334a81@163.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Wed, 11 Nov 2026 10:53:03 +0000
Precedence: list

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: j1939: j1939_xtp_rx_cts_one() rejects standard CTS(0)
  frames
--

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

can: j1939: reject a CTS that names more packets than the message has

This commit aborts a J1939 session with J1939_XTP_ABORT_ECTS_TOO_BIG in
j1939_xtp_rx_cts_one() when the received CTS packet number exceeds
session->pkt.total.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index fc2840f334b5b..377991d36dc6a 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1512,6 +1512,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session=
, struct sk_buff *skb)
>  		goto out_session_cancel;
>  	else if (dat[1] > session->pkt.block /* 0xff for etp */)
>  		goto out_session_cancel;
> +	else if (pkt > session->pkt.total) {
> +		/* The responder asks for more packets than the message has.
> +		 * pkt - 1 becomes pkt.tx_acked below, and the transmitter
> +		 * reports that value in an ETP.CM_DPO, where it would name a
> +		 * packet past the end of the message.
> +		 */
> +		err =3D J1939_XTP_ABORT_ECTS_TOO_BIG;
> +		goto out_session_cancel;
> +	}
> =20
>  	/* set packet counters only when not CTS(0) */
>  	session->pkt.tx_acked =3D pkt - 1;

[Severity: Medium]
Does this regression reject standard CTS(0) connection-hold frames?

Per SAE J1939-21 and ISO 11783-6, a CTS(0) frame (dat[1] =3D=3D 0) sets the
next packet number field to 0xff for TP or 0xffffff for ETP.  Because this
check runs unconditionally even when dat[1] is 0, any session with
session->pkt.total smaller than 0xff (TP) or 0xffffff (ETP) will abort with
J1939_XTP_ABORT_ECTS_TOO_BIG upon receiving a CTS(0) instead of reaching
the CTS(0) timeout handler further down in j1939_xtp_rx_cts_one():

net/can/j1939/transport.c:j1939_xtp_rx_cts_one() {
	...
	if (dat[1]) {
		...
	} else {
		/* CTS(0) */
		j1939_tp_set_rxtimeout(session, 550);
	}
}

Also note that updating session->pkt.tx_acked, session->pkt.last, and
session->pkt.tx right below the new check when dat[1] =3D=3D 0 contradicts =
the
"set packet counters only when not CTS(0)" comment unless both the new
bounds check and the packet counter updates are gated on dat[1] !=3D 0.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261111-j1939-edpo=
-v9-v9-1-e779ca334a81@163.com?part=3D4