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

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

[Alexander Hölzl]

There is a potential race condition in segmented messages using
the BAM protocol. When initiating a tx BAM session, first
the BAM itself is sent and then the txtimer is scheduled.
The tx timeout for BAM is currently hardcoded to 50ms.
As soon as the loopbacked BAM frame, acting as a tx
acknowledgment is received, the session->last_cmd field
is set to J1939_TP_CMD_BAM.
When the txtimer elapses the function j1939_xtp_txnext_transmiter
is called, which checks session->last_cmd and continues with
sending a data frame only if session->last_cmd is set to J1939_TP_CMD_BAM.
If there is a high busload and the BAM frame is stuck in the controller
for some time, the txtimer might elapse before the looped back frame is
received and the session will stall.
This problem becomes very obvious when changing the tx timeout
for BAMs to the lowest value allowed by the specification of 10ms.
This patch fixes the problem by moving the tx timer scheduling for
BAM transmissions such that the timer is scheduled when the tx ack
is received. This also fixes the problem that when consecutive
data frames are stuck in the controller they will be sent
without any delay.

Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
 net/can/j1939/transport.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5e6f..ec0679aded09 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -7,7 +7,6 @@
 //                         Marc Kleine-Budde <kernel@pengutronix.de>
 // Copyright (c) 2017-2019 Pengutronix,
 //                         Oleksij Rempel <kernel@pengutronix.de>
-
 #include <linux/can/skb.h>
 #include <net/can.h>
 
@@ -26,6 +25,9 @@
 #define J1939_TP_CMD_BAM 0x20
 #define J1939_TP_CMD_ABORT 0xff
 
+#define J1939_TP_BAM_FRAME_SPACING_MS 50
+#define J1939_TP_BAM_ECHO_TIMEOUT_MS 250
+
 #define J1939_ETP_CMD_RTS 0x14
 #define J1939_ETP_CMD_CTS 0x15
 #define J1939_ETP_CMD_DPO 0x16
@@ -766,7 +768,6 @@ static int j1939_session_tx_rts(struct j1939_session *session)
 
 	session->last_txcmd = dat[0];
 	if (dat[0] == J1939_TP_CMD_BAM) {
-		j1939_tp_schedule_txtimer(session, 50);
 		j1939_tp_set_rxtimeout(session, 250);
 	} else {
 		j1939_tp_set_rxtimeout(session, 1250);
@@ -866,12 +867,19 @@ static int j1939_session_tx_dat(struct j1939_session *session)
 		session->last_txcmd = 0xff;
 		pkt_done++;
 		session->pkt.tx++;
-		pdelay = j1939_cb_is_broadcast(&session->skcb) ? 50 :
-			j1939_tp_packet_delay;
 
-		if (session->pkt.tx < session->pkt.total && pdelay) {
-			j1939_tp_schedule_txtimer(session, pdelay);
+		/* For BAM transfer the tx timer is scheduled after receiving
+		 * the looped back frame as a tx ack. This means that here the
+		 * timer is only scheduled for directed transfers.
+		 */
+		pdelay = j1939_tp_packet_delay;
+		if (j1939_cb_is_broadcast(&session->skcb)) {
 			break;
+		} else {
+			if (session->pkt.tx < session->pkt.total && pdelay) {
+				j1939_tp_schedule_txtimer(session, j1939_tp_packet_delay);
+				break;
+			}
 		}
 	}
 
@@ -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);
+		}
 	} else {
 		if (!session->transmission) {
 			j1939_session_txtimer_cancel(session);
@@ -1961,6 +1973,12 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
 	} else if (remain) {
 		if (!session->transmission)
 			j1939_tp_set_rxtimeout(session, 750);
+		else if (j1939_cb_is_broadcast(&session->skcb)) {
+			/* cancel the txtimer to avoid leaking session references */
+			j1939_session_txtimer_cancel(session);
+			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
+			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
+		}
 	} else if (do_cts_eoma) {
 		j1939_tp_set_rxtimeout(session, 1250);
 		if (!session->transmission)
-- 
2.56.0

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

[sashiko-bot]

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

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

[netdev-bot+sashiko]

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

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

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] Refcount leak in j1939_xtp_rx_rts() (net/can/j1939/transport.c,
  new transmitter-BAM else-branch, lines 1819-1821).
- [Medium] A late echo restarts an aborted BAM transmission.
- [Medium] Race between the txtimer callback and BAM echo processing in
  j1939_xtp_rx_rts().
- [Low] The fix addresses a real, user-visible bug: a TP.BAM transmit
  session stalls and is aborted by the rx timeout when the BAM echo is…
- [Low] The first hunk (@@ -7,7 +7,6 @@) removes the blank line between
  the copyright comment block and `#include <linux/can/skb.h>` in…
- [Low] The new comment in j1939_session_tx_dat() (lines 871-874) and the
  commit message treat the looped-back frame as a "tx ack".
- [Low] The patch adds J1939_TP_BAM_ECHO_TIMEOUT_MS (250) but uses it
  only on the echo-receive side (j1939_xtp_rx_rts line 1821,…
- [Low] An attacker can stall a transmitter BAM session indefinitely with
  spoofed TP.DT frames.

--

can: j1939: fix potential race condition in BAM segmentation

This patch moves the txtimer scheduling for BAM transmissions out of the
transmit path and into the echo (looped back frame) receive path. The
next TP.DT is then only scheduled after the previous frame has been
echoed. This avoids a stall when the BAM echo arrives after the 50ms
txtimer has already fired.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..ec0679aded090 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -7,7 +7,6 @@
>  //                         Marc Kleine-Budde <kernel@pengutronix.de>
>  // Copyright (c) 2017-2019 Pengutronix,
>  //                         Oleksij Rempel <kernel@pengutronix.de>
> -
>  #include <linux/can/skb.h>
>  #include <net/can.h>

[Severity: Low]
This isn't a bug, but removing the blank line between the copyright block
and the includes looks unrelated to the BAM timing fix. Could it be
dropped from this patch?

[ ... ]

> @@ -766,7 +768,6 @@ static int j1939_session_tx_rts(struct j1939_session *session)
>  
>  	session->last_txcmd = dat[0];
>  	if (dat[0] == J1939_TP_CMD_BAM) {
> -		j1939_tp_schedule_txtimer(session, 50);

[Severity: Low]
Should this have a Fixes: tag? The 50ms txtimer arm removed here appears
to come from the original J1939 commit:

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

Users can see the stall, so a Cc: stable might also be appropriate.

>  		j1939_tp_set_rxtimeout(session, 250);

[Severity: Low]
This isn't a bug, but J1939_TP_BAM_ECHO_TIMEOUT_MS is only used on the
echo receive side, in j1939_xtp_rx_rts() and j1939_xtp_rx_dat_one().

The transmit side still uses a literal 250 in two places: here, and in
this line in j1939_session_tx_dat():

	if (pkt_done)
		j1939_tp_set_rxtimeout(session, 250);

Should these use the new constant too, so the two sides can't drift
apart?

>  	} else {
>  		j1939_tp_set_rxtimeout(session, 1250);

[ ... ]

> @@ -866,12 +867,19 @@ static int j1939_session_tx_dat(struct j1939_session *session)
>  		session->last_txcmd = 0xff;
>  		pkt_done++;
>  		session->pkt.tx++;
> -		pdelay = j1939_cb_is_broadcast(&session->skcb) ? 50 :
> -			j1939_tp_packet_delay;
>  
> -		if (session->pkt.tx < session->pkt.total && pdelay) {
> -			j1939_tp_schedule_txtimer(session, pdelay);
> +		/* For BAM transfer the tx timer is scheduled after receiving
> +		 * the looped back frame as a tx ack. This means that here the
> +		 * timer is only scheduled for directed transfers.
> +		 */

[Severity: Low]
Is the looped back frame always a tx ack? In can_send(), devices without
IFF_ECHO get a software clone. It is passed to netif_rx() as soon as
dev_queue_xmit() returns:

net/can/af_can.c:can_send() {
    ...
	err = dev_queue_xmit(skb);
    ...
	if (newskb)
		netif_rx(newskb);
    ...
}

That happens at enqueue time, not when the frame completes on the bus.
Examples are vcan without echo=1, slcan, can327, and drivers that only
set IFF_ECHO conditionally. On these devices, frames held in a qdisc or
driver queue can still go out back to back.

The commit message also says:

    This also fixes the problem that when consecutive
    data frames are stuck in the controller they will be sent
    without any delay.

Would it be more accurate to limit this claim, and this comment, to
IFF_ECHO devices?

> +		pdelay = j1939_tp_packet_delay;
> +		if (j1939_cb_is_broadcast(&session->skcb)) {
>  			break;

[ ... ]

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

[Severity: High]
Can this leak a session reference if the txtimer is already queued?

j1939_tp_schedule_txtimer() always takes a reference before calling
hrtimer_start():

void j1939_tp_schedule_txtimer(struct j1939_session *session, int msec)
{
	j1939_session_get(session);
	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);
}

If the timer is already pending, hrtimer_start() only moves its expiry.
The callback then runs once and drops only one of the two references.

j1939_xtp_rx_rts_session_active() only rejects sessions with
last_cmd != 0. The txtimer can be pending while last_cmd is still 0:

  - after j1939_tp_schedule_txtimer(session, 0) in j1939_sk_send_loop()
  - during the 10-25ms delayed activation in
    j1939_sk_queue_activate_next_locked()
  - during the -ENOBUFS retry re-arm in j1939_tp_txtimer()

A BAM with this session's SA and DA=0xff could arrive in that window in
two ways:

  - as a late echo from an earlier timed out session with the same
    addresses (j1939_session_match() doesn't compare the PGN)
  - as a spoofed frame, because j1939_can_recv() sets
    J1939_ECU_LOCAL_SRC from the SA alone, so a remote node or a local
    CAN_RAW socket can inject it

With a leaked reference, j1939_session_destroy() never runs. The priv
and netdev references are never dropped, so unregister_netdevice() could
hang waiting for the device.

The j1939_xtp_rx_dat_one() hunk in this patch cancels first, with the
comment "cancel the txtimer to avoid leaking session references". Should
this site also call j1939_session_txtimer_cancel() before re-arming?

> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);

[Severity: Medium]
Does this restart a session that has already been aborted?

If the BAM echo is delayed by more than 250ms (the high busload case in
the commit message), j1939_tp_rxtimer() aborts the session:

j1939_tp_rxtimer() {
    ...
			hrtimer_start(&session->rxtimer,
				      ms_to_ktime(J1939_XTP_ABORT_TIMEOUT_MS),
				      HRTIMER_MODE_REL_SOFT);
			__j1939_session_cancel(session, J1939_XTP_ABORT_TIMEOUT);
    ...
}

__j1939_session_cancel() sets state to J1939_SESSION_WAITING_ABORT and
sets session->err. It also calls j1939_sk_send_loop_abort(), which sets
sk->sk_err.

The session stays on active_session_list with last_cmd == 0, so
j1939_xtp_rx_rts_session_active() still lets the late echo through. This
branch then sets last_cmd to J1939_TP_CMD_BAM and arms the txtimer.
j1939_tp_set_rxtimeout() also replaces the abort-wait rxtimer.

j1939_xtp_txnext_transmiter() dispatches on last_cmd alone:

	case J1939_TP_CMD_BAM:
		ret = j1939_session_tx_dat(session);

So TP.DT frames would go out for a session that user space has already
been told failed. Each DT echo then re-arms both timers in
j1939_xtp_rx_dat_one() until the session completes.

Should the new rx path re-arms check session->state, under
active_session_list_lock, before restarting the timers? Without that
check, the rx path can also re-arm timers after j1939_session_cancel()
on another CPU has cancelled them.

[Severity: Medium]
Can this j1939_tp_set_rxtimeout() race with the one in
j1939_session_tx_rts()?

j1939_session_tx_rts() runs in the txtimer callback. It queues the BAM
via j1939_tp_tx_ctl() and only then calls
j1939_tp_set_rxtimeout(session, 250). The echo can be processed on
another CPU in that window, for example with the TX complete IRQ on a
different CPU, with RPS, or on PREEMPT_RT.

j1939_tp_set_rxtimeout() is not atomic:

	j1939_session_rxtimer_cancel(session);
	j1939_session_get(session);
	hrtimer_start(&session->rxtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);

Consider this interleaving:

CPU1 (echo, j1939_xtp_rx_rts)
  j1939_session_rxtimer_cancel()  -> nothing pending

CPU0 (txtimer, j1939_session_tx_rts)
  j1939_session_rxtimer_cancel()  -> nothing pending
  j1939_session_get()
  hrtimer_start()

CPU1
  j1939_session_get()
  hrtimer_start()

Two references are taken but only one rxtimer is queued, so one session
reference would leak.

j1939_xtp_rx_dat_one() cancels the txtimer first, and hrtimer_cancel()
waits for a running callback. Should this site do the same? The same
pattern already exists for directed RTS echoes, but this patch adds it
for BAM.

>  	} else {
>  		if (!session->transmission) {
>  			j1939_session_txtimer_cancel(session);
> @@ -1961,6 +1973,12 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session,
>  	} else if (remain) {
>  		if (!session->transmission)
>  			j1939_tp_set_rxtimeout(session, 750);
> +		else if (j1939_cb_is_broadcast(&session->skcb)) {
> +			/* cancel the txtimer to avoid leaking session references */
> +			j1939_session_txtimer_cancel(session);
> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);
> +			j1939_tp_set_rxtimeout(session, J1939_TP_BAM_ECHO_TIMEOUT_MS);
> +		}

[Severity: Low]
Should the timers only be re-armed here when the DT frame actually
advanced pkt.rx?

j1939_xtp_rx_dat() sends any TP.DT with J1939_ECU_LOCAL_SRC to the
transmitter session, and j1939_can_recv() sets that flag from the SA
alone.

A spoofed DT with a stale sequence number (packet < pkt.rx) passes the
bounds checks. A payload mismatch is only reported with
netdev_err_once(). pkt.rx doesn't move, so remain is still true.

This branch then pushes both the txtimer and the rxtimer out again. A
node on the bus, or a local CAN_RAW socket via loopback, could send such
frames more often than every 50ms. The session would then never transmit
or time out, and no error is reported until the socket is closed.

Before this patch, spoofed DT frames didn't touch the transmitter's
timers.

>  	} else if (do_cts_eoma) {
>  		j1939_tp_set_rxtimeout(session, 1250);
>  		if (!session->transmission)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011151216.118932-1-alexander.hoelzl%40gmx.net