Re: [PATCH v2] can: j1939: fix potential race condition in BAM segmentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: j1939: session reference leak in j1939_xtp_rx_rts on spoofed loopback
--

commit b1f46ca54f92cbc040e1810933f35ac82b494289
Author: Alexander Hölzl <alexander.hoelzl@gmx.net>

can: j1939: fix potential race condition in BAM segmentation

This patch resolves a race condition in segmented messages using the BAM
protocol by rescheduling the tx timer when the tx ack is received. It ensures
that consecutive data frames aren't sent without delay if they are stuck in the
controller.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1806,8 +1814,12 @@ static void j1939_xtp_rx_rts(struct j1939_priv *priv, struct sk_buff *skb,
>  	session->last_cmd = cmd;
>  
>  	if (cmd == J1939_TP_CMD_BAM) {
> -		if (!session->transmission)
> +		if (!session->transmission) {
>  			j1939_tp_set_rxtimeout(session, 750);
> +		} else {
> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
> +		}

[Severity: Medium]
Does this code leak a struct j1939_session reference if the timer was already
active?

If a BAM RTS transmission fails with -ENOBUFS (e.g., due to the CAN TX queue
being full), j1939_tp_txtimer() schedules session->txtimer to retry.

If a spoofed BAM loopback packet is received from the network during this
window, j1939_xtp_rx_rts() is called with transmitter = true.

Because it is the first received RTS for this session (session->last_cmd == 0),
it bypasses the active session abort check, processes the loopback, and calls
j1939_tp_schedule_txtimer() directly.

Since hrtimer_start() inside j1939_tp_schedule_txtimer() silently overwrites
the pending retry timer, could this abandon the session reference taken when
the retry timer was originally scheduled?

Should there be a call to j1939_session_txtimer_cancel(session) before
scheduling the new timer, similar to the one added in j1939_xtp_rx_dat_one()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011151216.118932-1-alexander.hoelzl@gmx.net?part=1