[PATCH v5 0/4] net: can: isotp-fixes

[PATCH v5 0/4] net: can: isotp-fixes

[Oliver Hartkopp via B4 Relay]

As sashiko-bot was not able to check the second patch this bundle is
re-posted with b4 preparation.

Add another two patches with fixes reported by sashiko-bot.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v5:
- patch 3: new: state change re-check after hrtimer_cancel()
- patch 4: new: handle state transistion with cmpxchg()
- Link to v4: https://patch.msgid.link/20260710-isotp-fixes-v4-0-2a4af437f61b@hartkopp.net

Changes in v4:
- patch 2: use a simpler method to reject a re-binding that can be implemented
  inside the lock: No waitqueue waiting just exit.
- Link to v3: https://patch.msgid.link/20260710-isotp-fixes-v3-0-08db68e27d0b@hartkopp.net

Changes in v3:
- patch 2: go back to initial idea without hard resetting the state machine and
  timers but let these come to a graceful end. Even a new bind() at netdev
  shutdown now leads to this graceful statemachine shutdown via the wait queue.
- Link to v2: https://patch.msgid.link/20260710-isotp-fixes-v2-0-bc57e26594b2@hartkopp.net

Changes in v2:
- patch 2: shutdown state machine to fix sashiko-bot complains
  Link: https://lore.kernel.org/linux-can/20260710094807.A50DD1F000E9@smtp.kernel.org/
- Link to v1: https://patch.msgid.link/20260710-isotp-fixes-v1-0-75a1d11d7df9@hartkopp.net

---
Oliver Hartkopp (4):
      can: isotp: use unconditional synchronize_rcu() in isotp_release()
      can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER
      can: isotp: fix race between RX timer and consecutive frame reception
      can: isotp: fix lock-free state transition in tx timer handler

 net/can/isotp.c | 115 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 83 insertions(+), 32 deletions(-)
---
base-commit: a635d6748234582ea287c5ffeae28b9b23f91c7e
change-id: 20260710-isotp-fixes-ead7480d67bf

Best regards,
--  
Oliver Hartkopp <socketcan@hartkopp.net>

[PATCH v5 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister()
and clears so->bound without waiting for a grace period. isotp_release()
uses so->bound to decide whether it needs to call synchronize_rcu()
before cancelling so->rxtimer, so when NETDEV_UNREGISTER runs first it
skips that synchronize_rcu() and can cancel the timer while an
in-flight isotp_rcv() is still executing and about to re-arm it via
isotp_send_fc(), leading to a use-after-free timer callback on the
freed socket.

sakisho-bot remarked a problem with rtnl_lock held in isotp_notify(),
therefore make isotp_release() always call synchronize_rcu() before
cancelling the timers, regardless of so->bound. This still closes the
original race (isotp_notify() clearing so->bound without waiting for
in-flight isotp_rcv() callers before isotp_release() cancels the RX
timer) without adding any RCU wait to the netdevice notifier path.

Fixes: 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")
Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/
Reported-by: Nico Yip <zdi-disclosures@trendmicro.com> (ZDI-CAN-31764)
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c48b4a818297..d30937345bcd 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1235,15 +1235,22 @@ static int isotp_release(struct socket *sock)
 
 				can_rx_unregister(net, dev, so->txid,
 						  SINGLE_MASK(so->txid),
 						  isotp_rcv_echo, sk);
 				dev_put(dev);
-				synchronize_rcu();
 			}
 		}
 	}
 
+	/* Always wait for a grace period before touching the timers below.
+	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
+	 * filters and cleared so->bound in isotp_notify() without waiting
+	 * for in-flight isotp_rcv() callers to finish, so this call must not
+	 * be skipped just because so->bound is already 0 here.
+	 */
+	synchronize_rcu();
+
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 	hrtimer_cancel(&so->rxtimer);
 
 	so->ifindex = 0;

-- 
2.53.0

Re: [PATCH v5 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[sashiko-bot]

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

Pre-existing issues:
- [High] A race condition between `isotp_release()` and a device network namespace move (or unregistration) causes `isotp_release()` to skip calling `can_rx_unregister()`. This leaves a dynamically allocated `struct receiver` linked in the CAN device's `dev_rcv_lists`, holding a dangling pointer to the socket `sk` which is subsequently freed. When the device receives packets in the new network namespace, this triggers a use-after-free on the socket.
--

--- Patch [1]: [PATCH v5 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit f75831bce5f2c912b988e63e3d43f06bda5b25c6
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

isotp_notify() unregisters the CAN filters and clears so->bound without waiting
for a grace period. This commit updates isotp_release() to always call
synchronize_rcu() before cancelling timers to prevent a use-after-free timer
callback on a freed socket.

Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);

[Severity: High]
This is a pre-existing issue, but does this code have a race condition when a
device is moved to another network namespace?

If a CAN device is moved to a new namespace, dev_get_by_index() called
earlier in isotp_release() will return NULL:

    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {
        if (isotp_register_rxid(so))
            can_rx_unregister(net, dev, so->rxid, ...

This causes isotp_release() to silently skip calling can_rx_unregister().

At the same time, isotp_release() prematurely removes the socket from
isotp_notifier_list. As a result, the notifier iteration skips the socket:

isotp_notifier() {
    ...
    list_for_each_entry(isotp_busy_notifier, &isotp_notifier_list, notifier) {
        spin_unlock(&isotp_notifier_lock);
        isotp_notify(isotp_busy_notifier, msg, dev);
    ...
}

This prevents the fallback unregistration in isotp_notify().

This leaves a dynamically allocated struct receiver linked in the CAN
device's dev_rcv_lists with a dangling pointer to the socket, which is
subsequently freed.

When the device receives packets in the new network namespace, could this
trigger a use-after-free on the socket?

> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v5-0-15174d82d6f1@hartkopp.net?part=1

[PATCH v5 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

isotp_release() looked up the bound network device via dev_get_by_index()
using the stored ifindex. During device unregistration the device is
unlisted from the ifindex hash before the NETDEV_UNREGISTER notifier
chain runs, so a concurrent isotp_release() could find no device, skip
can_rx_unregister() entirely, and still proceed to free the socket.
Since isotp_release() had already removed itself from the isotp
notifier list at that point, isotp_notify() would never get a chance to
clean up either, leaving a stale CAN filter that keeps pointing at the
freed socket.

Fix this the same way raw.c already does: hold a tracked reference to
the bound net_device in the socket (so->dev/so->dev_tracker) from
bind() onward instead of re-resolving it from the ifindex, and
serialize bind()/release() with rtnl_lock() so that so->dev is always
consistent with what the NETDEV_UNREGISTER notifier sees. so->dev
stays valid regardless of ifindex-hash unlisting, and is only ever
cleared by whichever of isotp_release()/isotp_notify() gets there
first, so the filter is always removed exactly once.

isotp_bind() now rejects a (re)bind with -EAGAIN while so->tx.state
isn't ISOTP_IDLE yet, so a timer left running by a prior
NETDEV_UNREGISTER can't act on a newly bound so->ifindex. Both checks
share the same lock_sock() section, so there is no window in which a
concurrent isotp_notify() clearing so->bound could be missed.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260707101420.47F261F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 87 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 28 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index d30937345bcd..e7597ca3e140 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -150,10 +150,12 @@ struct tpcon {
 
 struct isotp_sock {
 	struct sock sk;
 	int bound;
 	int ifindex;
+	struct net_device *dev;
+	netdevice_tracker dev_tracker;
 	canid_t txid;
 	canid_t rxid;
 	ktime_t tx_gap;
 	ktime_t lastrxcf_tstamp;
 	struct hrtimer rxtimer, txtimer, txfrtimer;
@@ -976,10 +978,18 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
 		if (err)
 			goto err_event_drop;
 	}
 
+	/* so->bound is only checked once above - a wakeup may have
+	 * unbound/rebound the socket meanwhile, so re-validate it
+	 */
+	if (!so->bound) {
+		err = -EADDRNOTAVAIL;
+		goto err_out_drop;
+	}
+
 	/* PDU size > default => try max_pdu_size */
 	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
 		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
 
 		if (newbuf) {
@@ -1217,32 +1227,34 @@ static int isotp_release(struct socket *sock)
 		spin_lock(&isotp_notifier_lock);
 	}
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);
 
+	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
-	if (so->bound) {
-		if (so->ifindex) {
-			struct net_device *dev;
-
-			dev = dev_get_by_index(net, so->ifindex);
-			if (dev) {
-				if (isotp_register_rxid(so))
-					can_rx_unregister(net, dev, so->rxid,
-							  SINGLE_MASK(so->rxid),
-							  isotp_rcv, sk);
-
-				can_rx_unregister(net, dev, so->txid,
-						  SINGLE_MASK(so->txid),
-						  isotp_rcv_echo, sk);
-				dev_put(dev);
-			}
-		}
+	/* remove current filters & unregister
+	 * tracked reference so->dev is taken at bind() time with rtnl_lock
+	 */
+	if (so->bound && so->dev) {
+		if (isotp_register_rxid(so))
+			can_rx_unregister(net, so->dev, so->rxid,
+					  SINGLE_MASK(so->rxid),
+					  isotp_rcv, sk);
+
+		can_rx_unregister(net, so->dev, so->txid,
+				  SINGLE_MASK(so->txid),
+				  isotp_rcv_echo, sk);
+		netdev_put(so->dev, &so->dev_tracker);
 	}
 
+	so->ifindex = 0;
+	so->bound = 0;
+	so->dev = NULL;
+
+	rtnl_unlock();
+
 	/* Always wait for a grace period before touching the timers below.
 	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
 	 * filters and cleared so->bound in isotp_notify() without waiting
 	 * for in-flight isotp_rcv() callers to finish, so this call must not
 	 * be skipped just because so->bound is already 0 here.
@@ -1251,13 +1263,10 @@ static int isotp_release(struct socket *sock)
 
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 	hrtimer_cancel(&so->rxtimer);
 
-	so->ifindex = 0;
-	so->bound = 0;
-
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
@@ -1308,17 +1317,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 
 	if (!addr->can_ifindex)
 		return -ENODEV;
 
+	rtnl_lock();
 	lock_sock(sk);
 
 	if (so->bound) {
 		err = -EINVAL;
 		goto out;
 	}
 
+	/* A transmission that outlived a previous binding (unbound by
+	 * NETDEV_UNREGISTER) may still be draining; the FC/echo watchdog
+	 * timers bound how long this takes. Checked together with
+	 * so->bound in the same lock_sock() section above, so there is no
+	 * window in which a concurrent isotp_notify() could be missed.
+	 */
+	if (so->tx.state != ISOTP_IDLE) {
+		err = -EAGAIN;
+		goto out;
+	}
+
 	/* ensure different CAN IDs when the rx_id is to be registered */
 	if (isotp_register_rxid(so) && rx_id == tx_id) {
 		err = -EADDRNOTAVAIL;
 		goto out;
 	}
@@ -1327,18 +1348,16 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
 	if (dev->type != ARPHRD_CAN) {
-		dev_put(dev);
 		err = -ENODEV;
-		goto out;
+		goto out_put_dev;
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
-		dev_put(dev);
 		err = -EINVAL;
-		goto out;
+		goto out_put_dev;
 	}
 	if (!(dev->flags & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
@@ -1352,20 +1371,29 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	/* register for echo skb's */
 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
 			isotp_rcv_echo, sk, "isotpe", sk);
 
-	dev_put(dev);
-
 	/* switch to new settings */
 	so->ifindex = ifindex;
 	so->rxid = rx_id;
 	so->txid = tx_id;
 	so->bound = 1;
 
+	/* bind() ok -> hold a reference for so->dev so that isotp_release()
+	 * can safely reach the device later, even if a concurrent
+	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
+	 */
+	so->dev = dev;
+	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
+
+out_put_dev:
+	/* remove potential reference from dev_get_by_index() */
+	dev_put(dev);
 out:
 	release_sock(sk);
+	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1564,11 +1592,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	struct sock *sk = &so->sk;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->ifindex != dev->ifindex)
+	if (so->dev != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
@@ -1580,14 +1608,16 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 						  isotp_rcv, sk);
 
 			can_rx_unregister(dev_net(dev), dev, so->txid,
 					  SINGLE_MASK(so->txid),
 					  isotp_rcv_echo, sk);
+			netdev_put(so->dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
+		so->dev = NULL;
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1643,10 +1673,11 @@ static int isotp_init(struct sock *sk)
 {
 	struct isotp_sock *so = isotp_sk(sk);
 
 	so->ifindex = 0;
 	so->bound = 0;
+	so->dev = NULL;
 
 	so->opt.flags = CAN_ISOTP_DEFAULT_FLAGS;
 	so->opt.ext_address = CAN_ISOTP_DEFAULT_EXT_ADDRESS;
 	so->opt.rx_ext_address = CAN_ISOTP_DEFAULT_EXT_ADDRESS;
 	so->opt.rxpad_content = CAN_ISOTP_DEFAULT_PAD_CONTENT;

-- 
2.53.0

Re: [PATCH v5 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing rx.state validation in isotp_bind() allows cross-interface data corruption and spurious timeouts.
--

--- Patch [2]: [PATCH v5 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER ---
commit 84cdeb4490291132fa60d9c7c68c759beafa051d
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

This patch fixes a use-after-free race by holding a tracked reference to
the bound net_device in the socket from bind() onward. It also serializes
bind() and release() with rtnl_lock() and adds a check to reject binding
if the transmission state is not idle.

Link: https://lore.kernel.org/linux-can/20260707101420.47F261F000E9@smtp.kernel.org/

> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1317,6 +1327,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> +	/* A transmission that outlived a previous binding (unbound by
> +	 * NETDEV_UNREGISTER) may still be draining; the FC/echo watchdog
> +	 * timers bound how long this takes. Checked together with
> +	 * so->bound in the same lock_sock() section above, so there is no
> +	 * window in which a concurrent isotp_notify() could be missed.
> +	 */
> +	if (so->tx.state != ISOTP_IDLE) {
> +		err = -EAGAIN;
> +		goto out;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but should we also validate so->rx.state here?

If a multi-frame ISOTP message is being received (ISOTP_WAIT_DATA state
with rxtimer armed) during a concurrent NETDEV_UNREGISTER, the socket is
unbound but so->rx.state and rxtimer remain active.

If the user quickly re-binds to a new interface, would consecutive CAN frames
on the newly bound interface be erroneously appended to the old buffer from
the deleted interface?

Could this also cause the legacy rxtimer to spuriously set sk_err = ETIMEDOUT
on the new connection?

>  	/* ensure different CAN IDs when the rx_id is to be registered */
>  	if (isotp_register_rxid(so) && rx_id == tx_id) {
>  		err = -EADDRNOTAVAIL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v5-0-15174d82d6f1@hartkopp.net?part=2

[PATCH v5 3/4] can: isotp: fix race between RX timer and consecutive frame reception

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

When receiving a Consecutive Frame (CF), hrtimer_cancel() is called to stop
the RX timer. However, if the timer handler (isotp_rx_timer_handler) has
already fired and is running concurrently on another CPU, hrtimer_cancel()
will wait for it to finish.

By the time it returns, the timer handler might have already reported an
ETIMEDOUT error and reset the RX state machine. If we blindly continue
processing the CF, we corrupt the newly reset or idle state.

Fix this by verifying that the state is still ISOTP_WAIT_DATA after
canceling the hrtimer. If it changed, the reception has already timed out,
so we drop the frame.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index e7597ca3e140..c0edbd35516c 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -575,10 +575,18 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
 		so->lastrxcf_tstamp = skb->tstamp;
 	}
 
 	hrtimer_cancel(&so->rxtimer);
 
+	/* isotp_rx_timer_handler() may have raced us for so->rx.state
+	 * while hrtimer_cancel() above waited for it to finish, already
+	 * reporting ETIMEDOUT and resetting the reception; don't process
+	 * this CF into a reassembly that has already been given up on.
+	 */
+	if (so->rx.state != ISOTP_WAIT_DATA)
+		return 1;
+
 	/* CFs are never longer than the FF */
 	if (cf->len > so->rx.ll_dl)
 		return 1;
 
 	/* CFs have usually the LL_DL length */

-- 
2.53.0

Re: [PATCH v5 3/4] can: isotp: fix race between RX timer and consecutive frame reception

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing state validation after hrtimer_cancel in the TX path allows resurrection of aborted transmissions and state machine corruption.
--

--- Patch [3]: [PATCH v5 3/4] can: isotp: fix race between RX timer and consecutive frame reception ---
commit 8ee7739c2adf82e961ff6ad45d8c572723ba74e7
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix race between RX timer and consecutive frame reception

When receiving a Consecutive Frame (CF), hrtimer_cancel() is called to stop
the RX timer. The patch verifies that the state is still ISOTP_WAIT_DATA after
canceling the hrtimer to avoid state machine corruption.

Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index e7597ca3e140b..c0edbd35516cc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -577,6 +577,14 @@ static int isotp_rcv_cf(struct sock *sk, struct canfd_frame *cf, int ae,
>  
>  	hrtimer_cancel(&so->rxtimer);
>  
> +	/* isotp_rx_timer_handler() may have raced us for so->rx.state
> +	 * while hrtimer_cancel() above waited for it to finish, already
> +	 * reporting ETIMEDOUT and resetting the reception; don't process
> +	 * this CF into a reassembly that has already been given up on.
> +	 */
> +	if (so->rx.state != ISOTP_WAIT_DATA)
> +		return 1;
> +

[Severity: High]
This is a pre-existing issue, but does a similar race condition exist in the
TX paths?

Looking at isotp_rcv_fc(), we call hrtimer_cancel() on the txtimer without
validating the TX state afterwards:

net/can/isotp.c:isotp_rcv_fc() {
    ...
    hrtimer_cancel(&so->txtimer);

    if ((cf->len < ae + FC_CONTENT_SZ) ||
    ...
}

Similarly in isotp_rcv_echo():

net/can/isotp.c:isotp_rcv_echo() {
    ...
    /* cancel local echo timeout */
    hrtimer_cancel(&so->txtimer);

    /* local echo skb with consecutive frame has been consumed */
    so->cfecho = 0;
    ...
}

If a flow control or echo CAN frame is received at the exact same time the
TX timer expires, could isotp_rcv_fc() or isotp_rcv_echo() block in
hrtimer_cancel() waiting for isotp_tx_timer_handler() to finish?

If the timer handler sets so->tx.state = ISOTP_IDLE and fails the socket,
could the receiver functions then blindly resume the transmission,
potentially resurrecting an aborted transfer and corrupting the state
machine?

Furthermore, since the state would briefly be ISOTP_IDLE, might a concurrent
sendmsg() succeed, resulting in two threads concurrently modifying the TX
buffer and state?

>  	/* CFs are never longer than the FF */
>  	if (cf->len > so->rx.ll_dl)
>  		return 1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v5-0-15174d82d6f1@hartkopp.net?part=3

[PATCH v5 4/4] can: isotp: fix lock-free state transition in tx timer handler

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit 051737439eae ("can: isotp: fix race between isotp_sendsmg() and
isotp_release()") introduced a lock-free state machine check
to prevent race conditions between the TX timer and concurrent state
updates. However, the original patch missed replacing the initial
state checks and left the late assignment of ISOTP_IDLE as a blind,
non-atomic write.

Fix this by properly sampling the initial state into 'old_state' and using
cmpxchg() to atomically move the state to ISOTP_IDLE. If the state changed
concurrently (e.g., due to an incoming echo or a new sendmsg), the timeout
is stale and we bail out safely without corrupting the state machine.

Fixes: 43a08c3bdac4cb ("can: isotp: isotp_sendmsg(): fix TX buffer concurrent access in isotp_sendmsg()")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c0edbd35516c..e49a58e188e6 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -920,24 +920,29 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
 {
 	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
 					     txtimer);
 	struct sock *sk = &so->sk;
+	u32 old_state = so->tx.state;
 
 	/* don't handle timeouts in IDLE or SHUTDOWN state */
-	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
+	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
+		return HRTIMER_NORESTART;
+
+	/* Only claim timeout if state is unchanged to avoid overwriting
+	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
+	 */
+	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
 		return HRTIMER_NORESTART;
 
 	/* we did not get any flow control or echo frame in time */
 
 	/* report 'communication error on send' */
 	sk->sk_err = ECOMM;
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk_error_report(sk);
 
-	/* reset tx state */
-	so->tx.state = ISOTP_IDLE;
 	wake_up_interruptible(&so->wait);
 
 	return HRTIMER_NORESTART;
 }
 

-- 
2.53.0

Re: [PATCH v5 4/4] can: isotp: fix lock-free state transition in tx timer handler

[sashiko-bot]

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

New issues:
- [Medium] Missing READ_ONCE() on lockless state read allows compiler load tearing to defeat cmpxchg protection

Pre-existing issues:
- [High] TOCTOU vulnerability in RX handlers allows state machine corruption and out-of-bounds reads
- [High] Unsafe teardown order in error path releases state machine before canceling async timers
- [High] Premature state release causes misattribution of timeout errors to concurrent transmissions
--

--- Patch [4]: [PATCH v5 4/4] can: isotp: fix lock-free state transition in tx timer handler ---
commit 1142eb2123699c3ef59ba2ca431beca7fc493cd1
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix lock-free state transition in tx timer handler

Commit 051737439eae ("can: isotp: fix race between isotp_sendsmg() and
isotp_release()") introduced a lock-free state machine check
to prevent race conditions between the TX timer and concurrent state
updates. However, the original patch missed replacing the initial
state checks and left the late assignment of ISOTP_IDLE as a blind,
non-atomic write.

Link: https://lore.kernel.org/linux-can/20260710142146.BDAE61F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c0edbd35516cc..e49a58e188e6b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -922,9 +922,16 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
>  					     txtimer);
>  	struct sock *sk = &so->sk;
> +	u32 old_state = so->tx.state;

[Severity: Medium]
Could this lockless state read be subject to compiler load tearing without a
READ_ONCE()?

If the compiler decides to reload so->tx.state directly into the cmpxchg
macro arguments under register pressure, and the state changes concurrently
(e.g., to ISOTP_SENDING), wouldn't the reloaded value cause cmpxchg to compare
the new state against itself?

Could this blind success erroneously abort the new transmission?

>  
>  	/* don't handle timeouts in IDLE or SHUTDOWN state */
> -	if (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
> +	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
> +		return HRTIMER_NORESTART;
> +
> +	/* Only claim timeout if state is unchanged to avoid overwriting
> +	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
> +	 */
> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)

[Severity: High]
This isn't a bug introduced by this patch, but does releasing the state
machine ownership here cause misattribution of timeout errors to concurrent
transmissions?

Since the state is transitioned to ISOTP_IDLE before setting sk->sk_err and
calling wake_up_interruptible(), could a concurrent sender claim ISOTP_SENDING
in this window?

If so, when the wait queue is woken, the original timed-out thread might wake
up, see the state is no longer ISOTP_IDLE, and go back to sleep.

Could the new sender later complete successfully but read the stale
sk->sk_err = ECOMM left by the timer, consuming the error, while the original
thread eventually wakes up and incorrectly reads success?

>  		return HRTIMER_NORESTART;
>  
>  	/* we did not get any flow control or echo frame in time */
> @@ -934,8 +941,6 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	if (!sock_flag(sk, SOCK_DEAD))
>  		sk_error_report(sk);
>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;

[Severity: High]
This is a pre-existing issue, but while reviewing the state machine changes,
is there a TOCTOU vulnerability in the RX handlers?

Looking at isotp_rcv_fc() in net/can/isotp.c:

    if (so->tx.state != ISOTP_WAIT_FC && ...)
        return 0;
    hrtimer_cancel(&so->txtimer);
    // ... blindly proceeds to send

The state is checked before the blocking hrtimer_cancel() and never
re-verified.

If a concurrent timeout aborts the transmission (setting state to ISOTP_IDLE)
while the RX thread is blocked in hrtimer_cancel(), and a new thread enters
isotp_sendmsg(), claims ISOTP_SENDING, and begins mutating so->tx.buf, what
happens when hrtimer_cancel() unblocks?

Would the RX handler blindly assume its earlier state check is still valid,
overwrite the state, and call isotp_send_cframe(), reading from the new
thread's partially initialized buffer?

A similar pattern appears in isotp_rcv_echo():

    if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
        return;
    hrtimer_cancel(&so->txtimer);
    // ... checks state but cfecho check was bypassed

The cfecho validation occurs before hrtimer_cancel(), allowing a concurrent
new transmission to slip past the check.

[Severity: High]
This is a pre-existing issue, but does isotp_sendmsg() have an unsafe
teardown order in its error path?

Looking at err_event_drop in isotp_sendmsg():

    err_event_drop:
        /* got signal: force tx state machine to be idle */
        so->tx.state = ISOTP_IDLE;
        hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);

If isotp_sendmsg() is interrupted by a signal, it sets so->tx.state =
ISOTP_IDLE before canceling the asynchronous timers.

Could a concurrent thread immediately enter isotp_sendmsg(), claim the state,
and start configuring so->tx?

If so, could the original thread's un-canceled txfrtimer then fire, see the
new thread's ISOTP_SENDING state, and prematurely call isotp_send_cframe() on
the new thread's incomplete buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v5-0-15174d82d6f1@hartkopp.net?part=4