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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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's main reason for having two hunks does not
  match the code.
- [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 patch fixes a kernel NULL pointer dereference that a peer can
  trigger: a NULL se_skb reaches j1939_sk_recv() from…
- [Low] The new comment in j1939_session_completed() justifies the NULL
  guard with "when a peer names a packet past the end of the message in…

--

can: j1939: validate the EDPO packet number

This patch adds a range check on the ETP.CM_DPO packet number, which
comes from the peer, in j1939_xtp_rx_dpo_one(). If the number is not
below pkt.total, the session is aborted with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. The patch also adds a NULL check on the
skb returned by j1939_session_skb_get() in j1939_session_completed(),
before it is passed to j1939_sk_recv().

Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3

> Reaching the dereference needs two things, and this patch keeps them as
> two separate changes because neither is sufficient alone:

[ ... ]

> The reproducer in the link was run three times in each configuration: the
> unpatched kernel panics 3/3, a kernel with only the NULL guard still
> panics 3/3, and this patch gives 0/3.

[Severity: Medium]
Is this accurate? For the sequence in the commit message, it looks like
each hunk would be enough on its own.

With only the NULL guard, pkt.dpo == 256 and pkt.total == 256, and
j1939_session_completed() calls j1939_session_skb_get():

net/can/j1939/transport.c:j1939_session_skb_get() {
	offset_start = session->pkt.dpo * 7;
	return j1939_session_skb_get_by_offset(session, offset_start);
}

For offset 1792 this returns NULL, and the guard then skips
j1939_sk_recv().

The other users of pkt.dpo don't seem able to fault either.
j1939_simple_txnext() already returns 0 on a NULL skb.
j1939_xtp_rx_dat_one() rejects packet > total, and a failed
j1939_session_skb_get_by_offset() there goes to out_session_cancel.

With only the range check, dpo < total gives
dpo * 7 <= (total - 1) * 7 < total_message_size. So the lookup always
lands inside the single reassembly skb that j1939_session_fresh_new()
allocates.

If the guard-only kernel really panics 3/3, could that be a different
crash from the one described here? Or should the test result be checked
again?

A smaller point in the same message:

> j1939_session_skb_get() for offset "pkt.dpo * 7" -- one full packet past
> the end of the queued skb -- gets NULL

The message is 1786 bytes, so 256 * 7 = 1792 is 6 bytes past the end of
the skb, not one full packet past it.

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260928193312.553632-1-mkl@pengutronix.de?part=3
> Assisted-by: LLM
> Signed-off-by: Chaosheng Qu <quchaosheng000406@163.com>

[Severity: Low]
Should this have a Fixes: tag and Cc: stable? Before this patch,
j1939_xtp_rx_dpo_one() stored j1939_etp_ctl_to_packet() into pkt.dpo
without any check. That leads to a NULL pointer dereference in
j1939_sk_recv(), reached from j1939_session_completed().

The commit message describes a local user on vcan. On a real CAN bus,
though, it looks like any remote node could send the crafted
ETP.CM_RTS, ETP.CM_DPO and ETP.DT frames. The only requirement seems to
be a local J1939 socket that matches the session, through
j1939_sk_recv_match() in j1939_xtp_rx_rts_session_new(). In that case no
local privileges are needed.

A Fixes: tag pointing at the commit that added the unchecked pkt.dpo
assignment would help stable triage.

> 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
> @@ -1243,9 +1243,18 @@ static void j1939_session_completed(struct j1939_session *session)
>  
>  	if (!session->transmission) {
>  		se_skb = j1939_session_skb_get(session);
> -		/* distribute among j1939 receivers */
> -		j1939_sk_recv(session->priv, se_skb);
> -		consume_skb(se_skb);
> +
> +		/* The offset "pkt.dpo * 7" is not necessarily covered by a
> +		 * queued skb, e.g. when a peer names a packet past the end of
> +		 * the message in an EDPO.  j1939_simple_txnext() already
> +		 * guards its call the same way; dropping the message is
> +		 * better than handing a NULL skb to j1939_sk_recv().
> +		 */

[Severity: Low]
This isn't a bug, but can the EDPO example in this comment still happen?
The range check this patch adds to j1939_xtp_rx_dpo_one() keeps pkt.dpo
below pkt.total for receive sessions. And (total - 1) * 7 always falls
inside the single reassembly skb.

Would it be clearer to describe the guard as defensive, or to name a
case that can still happen after this patch?

> +		if (se_skb) {
> +			/* distribute among j1939 receivers */
> +			j1939_sk_recv(session->priv, se_skb);
> +			consume_skb(se_skb);
> +		}
>  	}

[ ... ]

> @@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

> +	if (dpo >= session->pkt.total) {
> +		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 a second out of range EDPO leave the session stuck on
priv->active_session_list?

The first bad EDPO goes through j1939_session_cancel(). That arms the
abort rxtimer and moves the session to J1939_SESSION_WAITING_ABORT.

j1939_session_get_by_addr_locked() doesn't filter by state. So a second
bad EDPO within J1939_XTP_ABORT_TIMEOUT_MS reaches this branch again:

j1939_xtp_rx_dpo_one()
  j1939_session_timers_cancel()       <- cancels the pending abort rxtimer
  j1939_session_cancel()
    if (session->state >= J1939_SESSION_ACTIVE &&
        session->state < J1939_SESSION_WAITING_ABORT) {   <- false
            j1939_tp_set_rxtimeout(...);   <- not re-armed

The only automatic path that deactivates a WAITING_ABORT session is
j1939_tp_rxtimer(). With the timer cancelled, the session would keep its
reassembly skb (up to tp_max_packet_size), the netdev tracker and the
j1939_priv reference. These would be held until the peer sends an abort
or a new RTS, or the interface is torn down.

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

j1939_xtp_rx_rts_session_active() already works around this 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);
	}

Would something similar be needed here? The same timers_cancel plus
session_cancel pattern at out_session_cancel in j1939_xtp_rx_dat_one()
seems to have the same weakness.

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