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

[Marc Kleine-Budde]

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@xxxxxxxxxx>
> Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@xxxxxxxxxxxxxx?part=3
> Assisted-by: LLM
> Signed-off-by: Chaosheng Qu <quchaosheng000406@xxxxxxx>
> ---
> 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: signature.asc
Description: PGP signature