Re: [PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed retransmit tracking logic causes both spurious connection aborts and counter resets.
- [High] CTS hold messages bypass the retransmit limit, enabling indefinite connection persistence (DoS).
--

--- Patch [1]: [PATCH v4 1/3] j1939: restrict amount of consecutive retransmission requests ---
commit e2e0a35fca4d64ebcdd174aa2094ae405f7ecced
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

j1939: restrict amount of consecutive retransmission requests

This commit introduces a retransmit counter and aborts the segmented
data transfer if the retransmit limit is reached.

> diff --git a/net/can/j1939/j1939-priv.h b/net/can/j1939/j1939-priv.h
> index 81f58924b4acd..16867d8030922 100644
> --- a/net/can/j1939/j1939-priv.h
> +++ b/net/can/j1939/j1939-priv.h
[ ... ]
> @@ -285,6 +285,8 @@ struct j1939_session {
>  		unsigned int block;
>  		/* dpo - ETP.CM_DPO, Data Packet Offset */
>  		unsigned int dpo;
> +		/* retransmits - amount of received retransmit requests (including holds) */
> +		unsigned int retransmits;

[Severity: High]
If this counter is intended to include holds, will they actually be counted?

Looking at j1939_xtp_rx_cts_one() in transport.c, if j1939_cts_is_hold(skb)
is true, the function sets the hold timeout and returns early:

	if (j1939_cts_is_hold(skb)) {
		...
		j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
		return;
	}

Because this early return bypasses the new retransmits tracking logic,
could a peer keep a session open indefinitely by continuously sending
CTS hold messages?

>  	} pkt;
>  	struct hrtimer txtimer, rxtimer;
>  };
> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da7..6f999b18bbca5 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
[ ... ]
> @@ -1457,6 +1465,19 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>  	else if (dat[1] > session->pkt.block /* 0xff for etp */)
>  		goto out_session_cancel;
>  
> +	/* If the 'next packet number to be sent' in the CTS is smaller or
> +	 * equal to an already sent packet it is a retransmit request.
> +	 */
> +	if (session->pkt.tx >= pkt) {
> +		session->pkt.retransmits++;

[Severity: High]
Does this logic reliably track retransmits, given that session->pkt.tx
is updated asynchronously by txtimer?

If a peer drops a packet and requests the next one (forward progress),
but txtimer has already sent past that requested packet number, wouldn't
session->pkt.tx >= pkt evaluate to true? This seems like it could
spuriously increment the counter and abort valid transfers over lossy links.

Conversely, if a peer sends a duplicate CTS request before txtimer has sent
the newly requested packet, session->pkt.tx would still be pkt - 1. Wouldn't
that evaluate to false and cause the else block to reset the retransmits
counter to 0?

> +		if (session->pkt.retransmits >= J1939_CTS_MAX_NUM_TRANSMITS) {
> +			err = J1939_XTP_ABORT_FAULT;
> +			goto out_session_cancel;
> +		}
> +	} else {
> +		session->pkt.retransmits = 0;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706224548.100451-1-alexander.hoelzl@gmx.net?part=1