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

[netdev-bot+sashiko@kernel.org]

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