[PATCH net] can: j1939: transport: validate the DPO packet offset

[PATCH net] can: j1939: transport: validate the DPO packet offset

[Sun Yiqi]

j1939_xtp_rx_dpo_one() stores the packet offset from a received ETP DPO
command without checking it against the session's total packet count.
An offset greater than or equal to that count makes
j1939_session_skb_get() return NULL. A subsequent EOMA can then cause
j1939_session_completed() to pass the NULL skb to j1939_sk_recv(),
resulting in a NULL pointer dereference.

Reject an out-of-range offset before storing it and abort the session
with J1939_XTP_ABORT_BAD_EDPO_OFFSET. Keeping the previous valid offset
also prevents this lookup failure if an EOMA arrives while the session
is waiting for the abort to complete.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Sun Yiqi <sunyiqi2@huawei.com>
---
 net/can/j1939/transport.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..bda50432a72e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1823,14 +1823,22 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 				 struct sk_buff *skb)
 {
 	const u8 *dat = skb->data;
+	unsigned int packet;
 
 	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
 		return;
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	packet = j1939_etp_ctl_to_packet(skb->data);
+	if (packet >= session->pkt.total) {
+		j1939_session_timers_cancel(session);
+		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
+		return;
+	}
+
 	/* transmitted without problems */
-	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	session->pkt.dpo = packet;
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 
-- 
2.51.0.windows.1

Re: [PATCH net] can: j1939: transport: validate the DPO packet offset

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] can: j1939: transport: validate the DPO packet offset

[Oleksij Rempel]

On Thu, Nov 05, 2026 at 02:59:31PM +0800, Sun Yiqi wrote:
> j1939_xtp_rx_dpo_one() stores the packet offset from a received ETP DPO
> command without checking it against the session's total packet count.
> An offset greater than or equal to that count makes
> j1939_session_skb_get() return NULL. A subsequent EOMA can then cause
> j1939_session_completed() to pass the NULL skb to j1939_sk_recv(),
> resulting in a NULL pointer dereference.
> 
> Reject an out-of-range offset before storing it and abort the session
> with J1939_XTP_ABORT_BAD_EDPO_OFFSET. Keeping the previous valid offset
> also prevents this lookup failure if an EOMA arrives while the session
> is waiting for the abort to complete.
> 
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sun Yiqi <sunyiqi2@huawei.com>

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you for your work!

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net] can: j1939: transport: validate the DPO packet offset

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

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] When a second out-of-range ETP DPO arrives, the new bad-offset
  branch in j1939_xtp_rx_dpo_one() (net/can/j1939/transport.c) cancels…

--

can: j1939: transport: validate the DPO packet offset

This patch makes j1939_xtp_rx_dpo_one() check the packet offset from a
received ETP DPO against session->pkt.total. An offset that is out of
range is rejected and the session is aborted with
J1939_XTP_ABORT_BAD_EDPO_OFFSET. This prevents a NULL skb from reaching
j1939_sk_recv() when a later EOMA arrives.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..bda50432a72ea 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1823,14 +1823,22 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,

[ ... ]

> +	packet = j1939_etp_ctl_to_packet(skb->data);
> +	if (packet >= session->pkt.total) {
> +		j1939_session_timers_cancel(session);
> +		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
> +		return;
> +	}

[Severity: High]
What happens if a second out-of-range DPO arrives while the session is
already in J1939_SESSION_WAITING_ABORT?

After the first bad DPO, j1939_session_cancel() arms the abort rxtimer
and moves the session to WAITING_ABORT. The session stays on
priv->active_session_list. j1939_session_get_by_addr_locked() does not
filter on state, so j1939_xtp_rx_dpo() can still find it.

If a second bad DPO comes in before J1939_XTP_ABORT_TIMEOUT_MS expires,
this branch runs:

j1939_xtp_rx_dpo_one()
  j1939_session_timers_cancel()
    j1939_session_rxtimer_cancel()
      hrtimer_cancel(&session->rxtimer)   <- pending abort timer removed
      j1939_session_put(session)
  j1939_session_cancel()
    if (session->state >= J1939_SESSION_ACTIVE &&
        session->state < J1939_SESSION_WAITING_ABORT) {
        ...                              <- skipped, already WAITING_ABORT
    }

j1939_tp_rxtimer() is the only timeout path that deactivates a session
in WAITING_ABORT:

    if (session->state == J1939_SESSION_WAITING_ABORT) {
        ...
        j1939_session_deactivate_activate_next(session);

Once its timer has been cancelled, that path cannot run. Does the
session then stay on the active list for good, still holding its
reassembly skb and its netdev and j1939_priv references?

Every repeated bad DPO would also queue another J1939_ERRQUEUE_RX_ABORT.

Before this patch, a DPO in WAITING_ABORT only called
j1939_tp_set_rxtimeout(session, 750). That re-armed rxtimer, so the
session was still deactivated later.

j1939_xtp_rx_rts_session_active() already documents this hazard and
works around it:

    } 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);

That workaround came from commit 1809c82aa073 ("net: can: j1939:
j1939_xtp_rx_rts_session_active(): deactivate session upon receiving
the second rts"). It fixed a syzbot refcount leak that showed up as
"unregister_netdevice: waiting for vcan0 to become free".

Any of these could avoid the problem:

- Return early when session->state >= J1939_SESSION_WAITING_ABORT.

- Cancel only the txtimer here. j1939_session_cancel() already cancels
  and re-arms rxtimer through j1939_tp_set_rxtimeout().

- Force j1939_session_deactivate_activate_next() when the state is
  already WAITING_ABORT, as the RTS path does.

The out_session_cancel paths in j1939_xtp_rx_cts_one() and
j1939_xtp_rx_dat_one() use the same cancel-then-noop sequence, so a
shared helper might be a better fix.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105065931.1302-1-sunyiqi2%40huawei.com