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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [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…
- [Low] The commit message reports that a kernel with only the NULL guard
  in j1939_session_completed() "still panics 3 out of 3 runs", and uses…

Pre-existing issues:
- [Low] j1939_xtp_rx_dat_one() computes `packet = (dat[0] - 1 +
  session->pkt.dpo)` and rejects only `packet > session->pkt.total`, so…

--

can: j1939: validate the EDPO packet number

This patch checks the ETP.CM_DPO packet number against pkt.total in
j1939_xtp_rx_dpo_one(). When the number is out of range, it aborts the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET. It also adds a NULL check for
the skb returned by j1939_session_skb_get() in j1939_session_completed().

> Both roles tolerate the check because 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.

[Severity: Medium]

Where is the cts_packet <= pkt.total bound enforced? In
j1939_xtp_rx_cts_one(), the peer's 24-bit packet number is rejected only
when it is 0 or when dat[1] is larger than pkt.block:

net/can/j1939/transport.c:j1939_xtp_rx_cts_one() {
    ...
	if (!pkt)
		goto out_session_cancel;
	else if (dat[1] > session->pkt.block /* 0xff for etp */)
		goto out_session_cancel;

	/* set packet counters only when not CTS(0) */
	session->pkt.tx_acked = pkt - 1;
	j1939_session_skb_drop_old(session);
	session->pkt.last = session->pkt.tx_acked + dat[1];
	if (session->pkt.last > session->pkt.total)
		/* safety measure */
		session->pkt.last = session->pkt.total;
    ...
}

Only pkt.last is clamped. tx_acked is not, and the transmitter copies it
straight into its EDPO:

net/can/j1939/transport.c:j1939_session_tx_dpo() {
    ...
	session->pkt.dpo = session->pkt.tx_acked;
	pkt = session->pkt.dpo;
    ...
}

So a peer that sends an ETP.CM_CTS with a packet number above pkt.total
makes the local transmitter put an EDPO >= pkt.total on the bus. That
EDPO is looped back through j1939_xtp_rx_dpo(priv, skb, true) into
j1939_xtp_rx_dpo_one() on the transmit session.

There the new check aborts the transmit session with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. That is a receiver-side abort code, and it
is sent back to the peer and reported to the sending socket.

No memory-safety problem was found on the transmitter side.

Should j1939_xtp_rx_cts_one() also check the CTS packet number against
pkt.total? That would make the invariant in the commit message hold.

> On v7.3.0-rc5 under QEMU:
>
>   - an unpatched kernel panics 3 out of 3 runs, in j1939_sk_recv():
>       BUG: kernel NULL pointer dereference, address: 0000000000000018
>       RIP: 0010:j1939_sk_recv+0xaf/0x150
>   - with only the NULL guard in j1939_session_completed() (i.e. without
>     the EDPO range check) it still panics 3 out of 3 runs, so the guard
>     alone does not close the hole and the range check is the actual fix

[Severity: Low]

Is there an oops trace for the guard-only kernel? The only crash shown is
j1939_session_completed() passing a NULL se_skb to j1939_sk_recv(), and the
new guard removes exactly that dereference.

Here is the reproducer sequence with only the guard applied:

- The closing DT gives packet 255.
- The lookup at offset 1785 succeeds and pkt.rx reaches 256.
- The EOMA loopback then runs j1939_session_completed().
- Its lookup at offset 1792 returns NULL, and the guard now skips it.

The other users of a pkt.dpo-based lookup already handle NULL:

- j1939_simple_txnext() returns 0.
- j1939_xtp_rx_dat_one() goes to out_session_cancel.
- j1939_session_tx_dat() returns -ENOBUFS.

Where does the remaining panic come from? If there is no trace for it,
could this part of the commit message be corrected or dropped? The range
check itself still looks worth having.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..065b4f71e9af8 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
>  				 struct sk_buff *skb)
>  {
>  	const u8 *dat = skb->data;
> +	unsigned int dpo;
>  
>  	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
>  		return;
>  
>  	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>  
> +	dpo = j1939_etp_ctl_to_packet(dat);
> +

[ ... ]

> +	if (dpo >= session->pkt.total) {

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch. The new
check treats any index >= pkt.total as out of range, but the related check
in j1939_xtp_rx_dat_one() only rejects packet > pkt.total:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	packet = (dat[0] - 1 + session->pkt.dpo);
	if (packet > session->pkt.total ||
	    (session->pkt.rx + 1) > session->pkt.total) {
    ...
}

Even with dpo limited to pkt.total - 1, the peer controls dat[0]. A value
of 2 still gives packet == pkt.total. The next
j1939_session_skb_get_by_offset(session, packet * 7) returns NULL and the
session is cancelled through out_session_cancel, so this does not crash.

Should j1939_xtp_rx_dat_one() use the same >= bound, for consistency?

> +		netdev_warn(session->priv->ndev,
> +			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
> +			    __func__, session, dpo, session->pkt.total);
> +		j1939_session_timers_cancel(session);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: Medium]

Can this leave a receive session stuck in J1939_SESSION_WAITING_ABORT? The
new branch cancels both timers without checking the session state.

First out-of-range EDPO:

  j1939_session_timers_cancel()
  j1939_session_cancel()
    state is ACTIVE
    j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS)
    __j1939_session_cancel() -> state = J1939_SESSION_WAITING_ABORT

Second out-of-range EDPO with the same PGN:

  j1939_session_get_by_addr_locked() does not filter by state
  j1939_xtp_rx_cmd_bad_pgn() lets it through
  j1939_session_timers_cancel()
    hrtimer_cancel(&session->rxtimer)  <- abort timeout removed
  j1939_session_cancel()
    state is WAITING_ABORT, so rxtimer is not re-armed

j1939_session_cancel() only re-arms the timer in this case:

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

The WAITING_ABORT branch in j1939_tp_rxtimer() is the only automatic way to
deactivate the session.

The receiver's own looped-back ABORT is only passed to
j1939_xtp_rx_abort(priv, skb, true), which matches transmitter sessions. So
the session and its reassembly skb stay on priv->active_session_list until
one of these happens:

- the peer sends an ABORT
- the peer sends an RTS with the same PGN
- the netdev goes down

Until then, an RTS for a different PGN on that sa/da pair is rejected with
BUSY.

Before this patch, a DPO that arrived in WAITING_ABORT went through
j1939_tp_set_rxtimeout(session, 750), which re-armed rxtimer.

The out_session_cancel paths in j1939_xtp_rx_dat_one() and
j1939_xtp_rx_cts_one() already have this exposure. This branch adds another
instance of it.

j1939_xtp_rx_rts_session_active() already handles the same problem:

		} else if (session->state == J1939_SESSION_WAITING_ABORT) {
			/* Force deactivation for the receiver.
			 * If we rely on the timer starting in j1939_session_cancel,
			 * a second RTS call here will cancel that timer and fail
			 * to restart it because the state is already WAITING_ABORT.
			 */
			j1939_session_deactivate_activate_next(session);
		}

Should the new EDPO branch do the same, or skip the timer cancel when the
session is already in WAITING_ABORT?

>  
>  	/* transmitted without problems */
> -	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
> +	session->pkt.dpo = dpo;
>  	session->last_cmd = dat[0];
>  	j1939_tp_set_rxtimeout(session, 750);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930055942.216879-1-quchaosheng000406%40163.com