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

[Quchaosheng]

On Sat, 03 Oct 2026, netdev-bot+sashiko@xxxxxxxxxx wrote:

> Critical: 0 - High: 0 - Medium: 2 - Low: 1 (1)
>
> - [Medium] The commit message says the transmitter role is unaffected by
>   the new check: "a transmitter derives the offset from pkt.tx_acked,...
> - [Medium] The new EDPO abort branch in j1939_xtp_rx_dpo_one() calls
>   j1939_session_timers_cancel() and then j1939_session_cancel() without...

Thanks for the review. One of the two Medium items is a real problem with
my commit message and one I believe is a false positive. Details below.

On the first one: you are right and my summary was wrong. I wrote that a
transmitter derives the offset from pkt.tx_acked, which is "cts_packet - 1"
with cts_packet <= pkt.total, so the largest value it ever announces is
pkt.total - 1. Nothing in j1939_xtp_rx_cts_one() enforces that bound. The
only checks there are:

	if (!pkt)
		goto out_session_cancel;
	else if (dat[1] > session->pkt.block)
		goto out_session_cancel;

	session->pkt.tx_acked = pkt - 1;

and for ETP pkt comes from (dat[4] << 16) | (dat[3] << 8) | dat[2], so a
peer can set tx_acked past pkt.total. The clamp a few lines further down
only bounds pkt.last:

	if (session->pkt.last > session->pkt.total)
		session->pkt.last = session->pkt.total;

tx_acked is left as received. So the sentence about the transmitter role
does not hold on what the code does today, and I will drop it rather than
try to prop it up. Whether j1939_xtp_rx_cts_one() should also reject
pkt > pkt.total is a separate question from this patch, and I would send
that as its own change rather than fold it in here.

On the second one I think the code is fine, but here is what I based it on
rather than just asserting it. j1939_xtp_rx_dpo_one() is called from
j1939_xtp_rx_dpo(), which takes the session with
j1939_session_get_by_addr() and always does j1939_session_put() on the way
out:

	j1939_xtp_rx_dpo_one(session, skb);
	j1939_session_put(session);

So returning early from the abort branch drops the reference the caller
took and there is no leak. Cancelling the timers before calling
j1939_session_cancel() is the same order the existing out_session_cancel
label in j1939_xtp_rx_cts_one() uses.

What I do not think is needed here is j1939_session_deactivate(). The
session is being aborted, not parked for reuse, and j1939_session_cancel()
already moves it out of the active range before scheduling the abort:

	if (session->state >= J1939_SESSION_ACTIVE &&
	    session->state < J1939_SESSION_WAITING_ABORT) {
		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
		__j1939_session_cancel(session, err);
	}

If you have a specific invariant in mind that deactivate() maintains on
this path, say which one and I will add the call. I would rather not add
it on a guess.

On the Low item about the commit message: fair, that sentence describes
the reproduction I ran before the range check existed and reads as a claim
about the patch. I will split the two so it is clear the NULL guard and
the DPO range check are separate steps and neither is presented as
sufficient on its own.

I will send a v3 with the commit message reworded so it no longer claims
something the CTS path does not enforce.

pw-bot: cr