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 5 potential
issue(s) to consider.

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

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).

- [High] The new range check in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c:1852-1858) can leave a receive session…
- [Medium] Part of the commit message's reasoning does not match the
  code.
- [Low] The patch fixes a NULL-pointer dereference that a remote CAN node
  can trigger through crafted ETP.CM_RTS/ETP.CM_DPO/ETP.DT frames, but…
- [Low] The new check in j1939_xtp_rx_dpo_one() also runs for transmit
  sessions, via the looped-back local DPO: j1939_tp_cmd_recv() calls…
- [Low] The new comment in j1939_session_completed() (lines 1247-1252)
  names 'when a peer names a packet past the end of the message in an…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch adds
  another caller that can reach it.

--

can: j1939: validate the EDPO packet number

This commit adds a range check on the ETP.CM_DPO packet number 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. It also adds a NULL check
in j1939_session_completed() on the skb returned by
j1939_session_skb_get(), before that skb 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 range check in j1939_xtp_rx_dpo_one(), which stops pkt.dpo from
>     leaving the message
>   - the NULL guard in j1939_session_completed(), which covers the offset
>     lookup coming up empty for any other reason
>
> 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.  The middle result is the point --
> it shows the guard does not close this hole on its own, so the range check
> is not redundant with it.

[Severity: Medium]
Is this description accurate? With only the NULL guard applied, the
reproducer sequence in the message seems to go like this:

  ETP.CM_DPO (packet 256) -> pkt.dpo = 256
  ETP.DT seq 0            -> packet 255, passes "packet > pkt.total"
  j1939_session_skb_get_by_offset(session, 1785) finds the 1786 byte skb
  pkt.rx reaches 256, the receiver sends EOMA

The loopback of that EOMA then runs:

  j1939_xtp_rx_eoma_one()
    j1939_session_completed()
      se_skb = j1939_session_skb_get(session);  /* offset 1792, NULL */
      if (se_skb) {                             /* j1939_sk_recv() skipped */

The other users of pkt.dpo on this path all handle a NULL lookup already:
j1939_simple_txnext(), j1939_session_tx_dat() and j1939_xtp_rx_dat_one().
j1939_sk_queue_activate_next() returns early for receive sessions with
sk == NULL.

The reverse case also seems to hold. With only the range check, dpo < total
gives dpo * 7 <= (total - 1) * 7 < total_message_size. The single reassembly
skb at offset 0 then always covers the lookup.

Where does the guard-only kernel panic in the 3/3 runs? Is there a second
crash site that the message doesn't describe?

Also, the message says the patch "keeps them as two separate changes", but
this is a single commit.

[ ... ]

> 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? A remote CAN node can trigger
the NULL dereference with crafted ETP.CM_RTS, ETP.CM_DPO and ETP.DT frames.

The unvalidated assignment in j1939_xtp_rx_dpo_one() and the outline of
j1939_session_completed() both come from:

  Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")

The unguarded se_skb lookup line was last changed in 7ac56e40d054.

The message describes reachability in terms of a local vcan interface. On
a real bus, can't any remote node send this sequence to a bound J1939
receiver?

> 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 the comment gives an EDPO past the end of the message
as its example. The range check added to j1939_xtp_rx_dpo_one() by this same
patch now rejects that case.

For receive sessions, pkt.dpo is written only in j1939_xtp_rx_dpo_one() and
starts at 0. The single reassembly skb sits at offset 0 with length
total_message_size. Since (total - 1) * 7 < total_message_size, the lookup
should always succeed after this patch.

Could the comment instead describe what the guard still protects against?

> +		if (se_skb) {
> +			/* distribute among j1939 receivers */
> +			j1939_sk_recv(session->priv, se_skb);
> +			consume_skb(se_skb);
> +		}
>  	}
>  
>  	j1939_session_deactivate_activate_next(session);
> @@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

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

[Severity: Low]
This check also runs for transmit sessions, through the looped-back local
DPO. When LOCAL_SRC is set, j1939_tp_cmd_recv() calls
j1939_xtp_rx_dpo(priv, skb, true).

The transmit side can still produce a pkt.dpo past the end of the message.
j1939_xtp_rx_cts_one() only rejects pkt == 0, and it doesn't compare pkt
against pkt.total:

	if (!pkt)
		goto out_session_cancel;
	...
	session->pkt.tx_acked = pkt - 1;

j1939_session_tx_dpo() then sends that value:

	session->pkt.dpo = session->pkt.tx_acked;

When the DPO echoes back, the new check aborts the transmit session with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. That tells the peer our EDPO offset was
bad, but the actual fault was its oversized ETP.CM_CTS.

Would J1939_XTP_ABORT_ECTS_TOO_BIG, or a pkt.total check in
j1939_xtp_rx_cts_one(), be more accurate here? This doesn't crash, and the
local errno is EPROTO either way.

It also means the statement that the check "stops pkt.dpo from leaving the
message" only holds for receive sessions.

> +		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: High]
Can this leave a receive session stuck forever? Suppose a peer sends two
ETP.CM_DPO frames, both with an offset >= pkt.total, less than 500 ms apart.

The first bad DPO:

  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()
    j1939_session_cancel()
      j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS)
      __j1939_session_cancel()       /* state -> WAITING_ABORT */

j1939_session_get_by_addr_locked() doesn't filter on state, so the second
bad DPO still finds the session:

  j1939_xtp_rx_dpo_one()
    j1939_session_timers_cancel()    /* cancels the pending abort rxtimer */
    j1939_session_cancel()           /* WAITING_ABORT, nothing re-armed */

For a receive session, the only code that force-deactivates it is the
WAITING_ABORT branch of j1939_tp_rxtimer(). The looped-back abort only
matches sessions with transmission == true.

That leaves the session on active_session_list with no timer armed. It keeps
its kref, the reassembly skb, the priv reference and the netdev_hold()
reference from j1939_session_new().

If the last J1939 socket is then closed, __j1939_rx_release() clears priv
from the netdev. After that, j1939_netdev_notify() can't reach the session,
and unregister_netdevice would wait forever on the netdev reference.

Before this patch, a DPO in WAITING_ABORT went through
j1939_tp_set_rxtimeout(session, 750), which re-armed the timer, so this path
used to recover on its own. A similar stranding was already possible through
the j1939_xtp_rx_dat_one() error path.

j1939_xtp_rx_rts_session_active() already works around this exact problem:

		j1939_session_timers_cancel(session);
		j1939_session_cancel(session, J1939_XTP_ABORT_BUSY);
		if (session->transmission) {
			j1939_session_deactivate_activate_next(session);
		} 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 DPO path either skip the cancel when the session is already in
WAITING_ABORT, or force deactivation the way the RTS path does?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but the
patch adds one more caller that can reach it.

j1939_session_cancel() holds the session list lock (spin_lock_bh) while it
calls j1939_tp_set_rxtimeout(). That calls j1939_session_rxtimer_cancel(),
which uses hrtimer_cancel() and waits for a running callback.
j1939_tp_rxtimer() takes the same lock, directly in its else branch and
through j1939_session_deactivate() in the other branches.

Can these two deadlock?

  CPU A                                CPU C
  j1939_session_cancel()
    j1939_session_list_lock()
                                       j1939_tp_rxtimer()
                                         j1939_session_list_lock()
                                         /* spins on the lock */
    j1939_tp_set_rxtimeout()
      j1939_session_rxtimer_cancel()
        hrtimer_cancel()
        /* waits for CPU C's callback */

In the new branch, j1939_session_timers_cancel() runs before
j1939_session_cancel(), but without the lock. Another context can re-arm the
rxtimer in between. Examples are a concurrent valid DPO or DAT frame, or a
txtimer-driven CTS or EOMA calling j1939_tp_set_rxtimeout().

j1939_tp_txtimer() also calls j1939_session_cancel() on -EOVERFLOW and in
its default case without cancelling the timers first.

Under the same lock, j1939_cancel_active_session() uses
hrtimer_try_to_cancel() instead:

			if (hrtimer_try_to_cancel(&session->rxtimer) == 1)
				j1939_session_put(session);

Would j1939_session_cancel() need a similar non-waiting cancel?

>  	/* 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/20261009105410.300321-1-quchaosheng000406%40163.com