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

[PATCH] 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 | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..d00f0c158bb8 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
@@ -754,7 +756,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);
@@ -854,12 +855,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;
+			}
 		}
 	}
 
@@ -1793,8 +1801,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);
@@ -1948,6 +1960,10 @@ 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)) {
+			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.43.0

Re: [PATCH] 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:
- [High] Remote-triggerable reference count leak of `j1939_session` due to un-cancelled `txtimer` when receiving duplicate or spoofed BAM data loopback frames.
--

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

can: j1939: fix potential race condition in BAM segmentation

This commit addresses a potential race condition in segmented messages using
the BAM protocol. It moves the TX timer scheduling so the timer is only
scheduled when the TX acknowledgment is received.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index df93d57907da7..d00f0c158bb8e 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c

[ ... ]

> @@ -1948,6 +1960,10 @@ 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)) {
> +			j1939_tp_schedule_txtimer(session, J1939_TP_BAM_FRAME_SPACING_MS);

[Severity: High]
Does this unconditionally leak a j1939_session reference if a duplicate or
spoofed BAM loopback frame arrives while the timer is already active?

If a duplicate DAT loopback frame is received, the code detects it and falls
through to the remain = true block, calling j1939_tp_schedule_txtimer() again:

net/can/j1939/transport.c:j1939_tp_schedule_txtimer() {
	j1939_session_get(session);
	hrtimer_start(&session->txtimer, ms_to_ktime(msec),
		      HRTIMER_MODE_REL_SOFT);
}

Since j1939_tp_schedule_txtimer() takes a session reference without cancelling
any existing timer, and the timer callback will only fire once, won't the extra
references permanently leak and prevent the session from being freed?

> +			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)

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