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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: j1939: stranded session in j1939_xtp_rx_dat_one() after bad EDPO
- [Medium] can: j1939: aborted ETP session in j1939_xtp_rx_dpo_one() still accepts data
--

commit f0380724094c96563fb623cd857d2412a9cce46d
Author: Chaosheng Qu <quchaosheng000406@163.com>

can: j1939: validate the EDPO packet number

This commit validates the packet number in ETP.CM_DPO (EDPO) against
session->pkt.total and aborts the session if the offset is out of range.
It also makes j1939_session_cancel() return whether the abort timer was
armed and adds a NULL check in j1939_session_completed().

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

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..8e171b7138594 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1158,13 +1158,16 @@ static void __j1939_session_cancel(struct j1939_session *session,
>  		j1939_sk_send_loop_abort(session->sk, session->err);
>  }
>  
> -static void j1939_session_cancel(struct j1939_session *session,
> +static bool j1939_session_cancel(struct j1939_session *session,
>  				 enum j1939_xtp_abort err)
>  {
> +	bool active = false;
> +
>  	j1939_session_list_lock(session->priv);
>  
>  	if (session->state >= J1939_SESSION_ACTIVE &&
>  	    session->state < J1939_SESSION_WAITING_ABORT) {
> +		active = true;
>  		j1939_tp_set_rxtimeout(session, J1939_XTP_ABORT_TIMEOUT_MS);
>  		__j1939_session_cancel(session, err);
>  	}
> @@ -1173,6 +1176,8 @@ static void j1939_session_cancel(struct j1939_session *session,
>  
>  	if (!session->sk)
>  		j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
> +
> +	return active;
>  }

[ ... ]

> @@ -1823,14 +1837,36 @@ 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);
> +
> +	/*
> +	 * EDPO is peer controlled.  It names the packet the announced window
> +	 * starts at, and j1939_xtp_rx_dat_one() places data at
> +	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
> +	 * value past the end moves pkt.dpo beyond the reassembled buffer and
> +	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
> +	 * no longer covered by any queued skb.
> +	 */
> +	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);
> +		if (!j1939_session_cancel(session,
> +					  J1939_XTP_ABORT_BAD_EDPO_OFFSET))
> +			j1939_session_deactivate_activate_next(session);
> +		return;
> +	}

[Severity: High]
Can an ETP.DT frame arriving right after a rejected ETP.CM_DPO strand the
struct j1939_session and leak its net_device references?

When the first out-of-range ETP.CM_DPO arrives on an active receiving ETP
session from a non-local peer, j1939_xtp_rx_dpo_one() calls
j1939_session_cancel(), which transitions session->state to
J1939_SESSION_WAITING_ABORT, arms session->rxtimer for 500 ms, and returns
true. The session remains on priv->active_session_list with session->last_cmd
and session->pkt.dpo unchanged (and for a non-local peer SA, the echoed abort
frame has J1939_ECU_LOCAL_SRC set and J1939_ECU_LOCAL_DST unset, so
j1939_tp_cmd_recv() does not deactivate the receiving session on loopback).

In ETP, ETP.CM_DPO is immediately followed by ETP.DT data frames (such as in
the 5-frame sequence in the commit description). When that ETP.DT frame
arrives within 500 ms and fails validation in j1939_xtp_rx_dat_one() (for
example, because session->last_cmd was not updated to J1939_ETP_CMD_DPO, or
because dat[0] == 0 with session->pkt.dpo == 0 evaluates packet to -1), it
jumps to out_session_cancel:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
 out_session_cancel:
	kfree_skb(se_skb);
	j1939_session_timers_cancel(session);
	j1939_session_cancel(session, abort);
	j1939_session_put(session);
}

Here j1939_session_timers_cancel() cancels the pending 500 ms abort rxtimer,
and j1939_session_cancel() sees session->state == J1939_SESSION_WAITING_ABORT,
does not re-arm rxtimer, and returns false. Because j1939_xtp_rx_dat_one()
(and similarly out_session_cancel in j1939_xtp_rx_cts_one()) ignores the
return value of j1939_session_cancel(), the session is left on
priv->active_session_list with no active timers, leaking struct j1939_session,
struct j1939_priv, and the net_device references if the socket is closed
before interface teardown.

> +
>  	/* 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);
>  

[Severity: Medium]
What happens if a remote peer follows an out-of-range ETP.CM_DPO with a valid
in-range ETP.CM_DPO and ETP.DT frames before the 500 ms abort timeout expires?

After the first out-of-range ETP.CM_DPO puts the receiving session into
J1939_SESSION_WAITING_ABORT, the session stays on priv->active_session_list
for 500 ms with its previous session->pkt.dpo and session->last_cmd intact.

Because neither j1939_xtp_rx_dpo_one(), j1939_xtp_rx_dat_one(), nor
j1939_session_completed() checks session->state or session->err:

net/can/j1939/transport.c:j1939_xtp_rx_dat_one() {
    ...
	switch (session->last_cmd) {
	case 0xff:
		break;
	case J1939_ETP_CMD_DPO:
		if (skcb->addr.type == J1939_ETP)
			break;
    ...
}

Wouldn't subsequent valid ETP.CM_DPO and ETP.DT frames received within that
500 ms window overwrite rxtimer, reassemble the payload, send ETP.CM_EOMA, and
deliver the already-aborted session's payload to user space via
j1939_session_completed() -> j1939_sk_recv()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020025036.74454-1-quchaosheng000406@163.com?part=1