Re: [PATCH net v9 4/4] can: j1939: reject a CTS that names more packets than the message has

[sashiko-bot@kernel.org]

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 = J1939_XTP_ABORT_ECTS_TOO_BIG;
> +		goto out_session_cancel;
> +	}
>  
>  	/* set packet counters only when not CTS(0) */
>  	session->pkt.tx_acked = 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] == 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] == 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] != 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-j1939-edpo-v9-v9-1-e779ca334a81@163.com?part=4