[PATCH v6 0/5] net: can: isotp-fixes

[PATCH v6 0/5] 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.

Fixed the remarks reported by sashiko-bot of V5 patchset.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v6:
- patch 2: check for rx state and tx state to be IDLE in isotp_bind() (sashiko-bot)
- patch 3: double check rx/tx states after all hrtimer_cancel() calls (sashiko-bot)
           move ISOTP_IDLE in err_event_drop for the same reason
- patch 4: use READ_ONCE() to prevent compiler load tearing as remarked by sashiko-bot
- Link to v5: https://patch.msgid.link/20260710-isotp-fixes-v5-0-15174d82d6f1@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 (5):
      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/TX timers and frame reception
      can: isotp: fix lock-free state transition in tx timer handler
      can: isotp: fix state machine corruption on signal interruption

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

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

[PATCH v6 1/5] 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 v6 1/5] 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] Race conditions between isotp_release/isotp_bind and NETDEV_UNREGISTER cause CAN filters to be leaked, leading to a Use-After-Free of the socket.
--

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

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

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)

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

[Severity: High]
This is a pre-existing issue, but can race conditions between isotp_release()
or isotp_bind() and NETDEV_UNREGISTER cause CAN filters to be leaked,
potentially leading to a use-after-free of the socket?

If a user creates a CAN interface, binds an ISOTP socket, and concurrently
unregisters the interface while closing the socket, isotp_release() removes
the socket from the notifier list:

net/can/isotp.c:isotp_release() {
        ...
        list_del(&so->notifier);
        spin_unlock(&isotp_notifier_lock);
        
        lock_sock(sk);
        ...
}

This blinds the NETDEV_UNREGISTER notifier. If isotp_release() executes before
isotp_notify(), the notifier skips the socket. When isotp_release() then tries
to clean up the filters, if the netdevice is already unlisted, it might skip
the can_rx_unregister() call. The socket would then be freed, but the active
CAN filter could still point to it.

A similar window exists in isotp_bind(), where the filters are registered
before so->ifindex is updated:

net/can/isotp.c:isotp_bind() {
        ...
        if (isotp_register_rxid(so))
                can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id), ...);
        
        can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id), ...);
        dev_put(dev);
        
        /* switch to new settings */
        so->ifindex = ifindex;
        so->rxid = rx_id;
        ...
}

If a concurrent NETDEV_UNREGISTER runs during this window, isotp_notify()
ignores the socket because it locklessly checks if (so->ifindex != dev->ifindex),
bypassing the unregister and leaking the filter when the socket is later
released.

Could this allow a local unprivileged user utilizing virtual CAN (vcan)
devices to trigger a use-after-free?

>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

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

[PATCH v6 2/5] 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|rx].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..44c044eb83e1 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 or reception that outlived a previous binding
+	 * (unbound by NETDEV_UNREGISTER) may still be draining; the FC/echo
+	 * and RX 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 || so->rx.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 v6 2/5] 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] The wait loop error path in `isotp_sendmsg()` resets the transmission state machine despite not owning it, which can silently corrupt another thread's active transmission and lead to an information leak.
--

--- Patch [2]: [PATCH v6 2/5] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER ---
commit 1c03594f024dd1735bb9c085227d4a04352dcd2f
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

This commit fixes a race condition during device unregistration by holding a
tracked reference to the bound net_device and serializing bind and release
with rtnl_lock().

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -978,6 +980,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			goto err_event_drop;

[Severity: High]
This is a pre-existing issue, but does this error path corrupt the state
machine for another thread?

If Thread A acquires the state (so->tx.state = ISOTP_SENDING) and starts
transmission, and Thread B calls isotp_sendmsg(), Thread B will fail the
cmpxchg() and block in wait_event_interruptible().

If Thread B receives a signal, it jumps to err_event_drop and forces 
so->tx.state = ISOTP_IDLE, even though Thread A still owns the active 
transmission.

Could Thread C then call isotp_sendmsg(), acquire the now-IDLE state
machine, and begin setting up a new transmission? If a delayed flow
control frame for Thread A's transmission arrives, it seems the state
machine could mistakenly transmit consecutive frames containing the newly
allocated, uninitialized memory from Thread C onto the CAN bus.

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

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

[PATCH v6 3/5] can: isotp: fix race between RX/TX timers and frame reception

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

When receiving a Consecutive Frame, Flow Control frame, or local echo
frame, hrtimer_cancel() is called to stop the corresponding watchdog
timer. If the timer handler was already running concurrently on another
CPU, hrtimer_cancel() waits for it to finish.

By the time it returns, the timer handler may have already reported a
timeout error and reset the state machine. Blindly continuing corrupts
the newly reset or idle state, and can let a concurrent sendmsg() claim
the same IDLE state at the same time.

Fix this by re-checking the state right after canceling the timer in all
three call sites (isotp_rcv_cf(), isotp_rcv_fc(), isotp_rcv_echo()). If
the state changed, the transfer has already timed out, so drop the frame
instead of resuming it.

For the same reason, isotp_sendmsg()'s err_event_drop path must cancel
so->txfrtimer/txtimer before setting so->tx.state to ISOTP_IDLE, not
after: otherwise a concurrent sendmsg() could claim ISOTP_SENDING and
start filling so->tx.buf while a still-armed timer from the aborted
transfer fires and sends a stale frame from it.

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 | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 44c044eb83e1..5ef2a610fc17 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -376,10 +376,19 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
 	    so->tx.state != ISOTP_WAIT_FIRST_FC)
 		return 0;
 
 	hrtimer_cancel(&so->txtimer);
 
+	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
+	 * hrtimer_cancel() above waited for it to finish, already reporting
+	 * the tx error and resetting the state; don't resume a tx job that
+	 * has already been given up on.
+	 */
+	if (so->tx.state != ISOTP_WAIT_FC &&
+	    so->tx.state != ISOTP_WAIT_FIRST_FC)
+		return 1;
+
 	if ((cf->len < ae + FC_CONTENT_SZ) ||
 	    ((so->opt.flags & ISOTP_CHECK_PADDING) &&
 	     check_pad(so, cf, ae + FC_CONTENT_SZ, so->opt.rxpad_content))) {
 		/* malformed PDU - report 'not a data message' */
 		sk->sk_err = EBADMSG;
@@ -575,10 +584,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 */
@@ -879,10 +896,18 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 	hrtimer_cancel(&so->txtimer);
 
 	/* local echo skb with consecutive frame has been consumed */
 	so->cfecho = 0;
 
+	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
+	 * hrtimer_cancel() above waited for it to finish, already reporting
+	 * the tx error and resetting the state; don't resume a tx job that
+	 * has already been given up on.
+	 */
+	if (so->tx.state != ISOTP_SENDING)
+		return;
+
 	if (so->tx.idx >= so->tx.len) {
 		/* we are done */
 		so->tx.state = ISOTP_IDLE;
 		wake_up_interruptible(&so->wait);
 		return;
@@ -1141,12 +1166,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	}
 
 	return size;
 
 err_event_drop:
-	/* got signal: force tx state machine to be idle */
-	so->tx.state = ISOTP_IDLE;
+	/* got signal: force tx state machine to be ISOTP_IDLE */
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
 err_out_drop:
 	/* drop this PDU and unlock a potential wait queue */
 	so->tx.state = ISOTP_IDLE;

-- 
2.53.0

Re: [PATCH v6 3/5] can: isotp: fix race between RX/TX timers and frame reception

[sashiko-bot]

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

New issues:
- [High] Moving `so->tx.state = ISOTP_IDLE` after `hrtimer_cancel()` in the `err_event_drop` path introduces a race condition with softirq receive handlers, leading to stray timers that can corrupt subsequent transmissions.
- [High] The delayed `so->tx.state = ISOTP_IDLE` assignment in the error path unconditionally clobbers the state, which can corrupt a new, concurrently running transfer.
- [High] The newly added TX state checks after `hrtimer_cancel()` in `isotp_rcv_fc()` and `isotp_rcv_echo()` are vulnerable to an ABA race condition, allowing stale frames to corrupt new transfers.

Pre-existing issues:
- [High] Missing cleanup of `so->cfecho` in `isotp_sendmsg()` error paths allows stale local echoes to corrupt subsequent transfers.
--

--- Patch [3]: [PATCH v6 3/5] can: isotp: fix race between RX/TX timers and frame reception ---
commit 4f481801d9d9059ae72ceaf6467d32f307eddbb5
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: isotp: fix race between RX/TX timers and frame reception

This patch fixes a race condition between hrtimer execution and frame
reception by checking the state after hrtimer_cancel(). It also reorders
timer cancellation and state reset in isotp_sendmsg()'s error path to
prevent concurrent sendmsg calls from corrupting state.

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 44c044eb83e12..5ef2a610fc172 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -378,6 +378,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  
>  	hrtimer_cancel(&so->txtimer);
>  
> +	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
> +	 * hrtimer_cancel() above waited for it to finish, already reporting
> +	 * the tx error and resetting the state; don't resume a tx job that
> +	 * has already been given up on.
> +	 */
> +	if (so->tx.state != ISOTP_WAIT_FC &&
> +	    so->tx.state != ISOTP_WAIT_FIRST_FC)
> +		return 1;
> +

[Severity: High]
Is this lockless state check vulnerable to an ABA transition?

While hrtimer_cancel() spins waiting for a timer handler, the timer handler
could reset the state to ISOTP_IDLE. If a concurrent isotp_sendmsg() from
another thread starts a new transfer during this window, it could locklessly
transition the state back to the exact same expected state
(e.g., ISOTP_WAIT_FIRST_FC).

When hrtimer_cancel() returns, this check would pass because the state
matches, but it would be evaluating a different transfer. Could this allow a
stale Flow Control frame to corrupt the new transfer?

[ ... ]
> @@ -881,6 +898,14 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  
> +	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
> +	 * hrtimer_cancel() above waited for it to finish, already reporting
> +	 * the tx error and resetting the state; don't resume a tx job that
> +	 * has already been given up on.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING)
> +		return;
> +

[Severity: High]
Could the same ABA race happen here?

If a timer resets the state to ISOTP_IDLE while hrtimer_cancel() waits,
and a concurrent isotp_sendmsg() starts a new transfer and sets the state to
ISOTP_SENDING, this check would pass for the new transfer.

Would this allow a stale local echo to erroneously resume the newly started
transfer?

[ ... ]
> @@ -1143,8 +1168,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	return size;
>  
>  err_event_drop:
> -	/* got signal: force tx state machine to be idle */
> -	so->tx.state = ISOTP_IDLE;
> +	/* got signal: force tx state machine to be ISOTP_IDLE */
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  err_out_drop:

[Severity: High]
Does delaying the state reset to after hrtimer_cancel() create a window
where concurrent RX softirqs can observe an active state?

If isotp_sendmsg() is interrupted by a signal, it now calls
hrtimer_cancel() while so->tx.state is still visibly active
(e.g., ISOTP_SENDING). If a CAN receive handler (like isotp_rcv_echo())
executes concurrently in softirq context, it locklessly checks
so->tx.state, sees it is active, and can re-arm the transmission timers.

When this error path completes, it sets the state to ISOTP_IDLE, leaving
the newly armed timers active. When a subsequent sendmsg() begins, could
these stray timers fire unexpectedly and corrupt the new transfer's
sequence?

[Severity: High]
Does the delayed assignment of ISOTP_IDLE in err_out_drop clobber
concurrently claimed state?

While hrtimer_cancel() waits for a timer handler to finish (which resets
the state to ISOTP_IDLE), a concurrent thread could call isotp_sendmsg(),
successfully claim the ISOTP_IDLE state, and begin a new transfer.

When the original thread finishes hrtimer_cancel(), it unconditionally
executes so->tx.state = ISOTP_IDLE (located under err_out_drop:). Could
this abruptly clobber the new transfer's state and expose its buffer to
concurrent overwrites?

[Severity: High]
This is a pre-existing issue, but does the error path fail to clean up
so->cfecho?

If a transmission is aborted via a signal, the state is forced to
ISOTP_IDLE but so->cfecho is not cleared and continues to hold the local
echo tag of the aborted frame.

When a subsequent sendmsg() sets the state to ISOTP_SENDING and begins
preparing the buffer, a stale echo could arrive. If isotp_rcv_echo() sees
a matching so->cfecho and an active ISOTP_SENDING state, could it process
the echo and initiate a transmission containing uninitialized heap data?

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

[PATCH v6 4/5] 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 5ef2a610fc17..0826ae837246 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -937,24 +937,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 = READ_ONCE(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 v6 4/5] can: isotp: fix lock-free state transition in tx timer handler

[sashiko-bot]

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

New issues:
- [High] Early state transition to ISOTP_IDLE in the TX timer handler introduces a race condition that can corrupt the error state of a concurrent transmission.

Pre-existing issues:
- [High] A signal during the `isotp_sendmsg()` wait loop can abort another thread's active transmission on a shared socket.
--

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

This commit introduces a cmpxchg to safely transition the state machine
to ISOTP_IDLE in the tx timer handler. It ensures the state is only updated
if it hasn't been changed concurrently.

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 5ef2a610fc172..0826ae8372467 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -939,9 +939,16 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)

[ ... ]

> +	/* 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 */
> @@ -951,8 +958,6 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	if (!sock_flag(sk, SOCK_DEAD))
>  		sk_error_report(sk);

[Severity: High]
Does this early state transition to ISOTP_IDLE introduce a race condition?

If the cmpxchg successfully sets the state to ISOTP_IDLE, another thread 
calling isotp_sendmsg() on a different CPU could see the IDLE state and
immediately start a new transmission before this timer handler completes.

If that happens, the timer thread would resume and execute sk->sk_err = ECOMM
and sk_error_report(sk), which might incorrectly taint the new transmission
with a communication error.

> -	/* 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 can a signal during the wait loop in
isotp_sendmsg() abort another thread's active transmission?

Looking at isotp_sendmsg(), if a concurrent sender fails to acquire the
state and waits:

net/can/isotp.c:isotp_sendmsg()
        err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
        if (err)
                goto err_event_drop;

If a signal interrupts this wait, the thread jumps to err_event_drop where
it unconditionally cancels the timers and resets the state:

err_event_drop:
        hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);
err_out_drop:
        so->tx.state = ISOTP_IDLE;

Since this thread was only waiting and never claimed the state machine, will
this forcefully cancel the active transmission of the thread that actually
owned it?

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

[PATCH v6 5/5] can: isotp: fix state machine corruption on signal interruption

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. Since this thread never owned the active transmission, it
abruptly aborts another thread's legitimate transfer, so return the
interruption error directly.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 0826ae837246..b8562f8c6de0 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1005,11 +1005,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			return -EADDRNOTAVAIL;
 
 		/* wait for complete transmission of current pdu */
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
 		if (err)
-			goto err_event_drop;
+			return err;
 	}
 
 	/* so->bound is only checked once above - a wakeup may have
 	 * unbound/rebound the socket meanwhile, so re-validate it
 	 */

-- 
2.53.0

Re: [PATCH v6 5/5] can: isotp: fix state machine corruption on signal interruption

[sashiko-bot]

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

Pre-existing issues:
- [High] Signal interruption of a transmission (e.g., during CAN_ISOTP_WAIT_TX_DONE) or a TX timeout aborts an active transmission by forcing state to ISOTP_IDLE but fails to clear `so->cfecho`. This exposes a race condition with delayed local loopback echoes that can leak uninitialized kernel heap memory or corrupt subsequent transmissions.
--

--- Patch [5]: [PATCH v6 5/5] can: isotp: fix state machine corruption on signal interruption ---
commit 0fb83cf387b5c9feb7682bf6c41fda7dbcb7b3d2
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix state machine corruption on signal interruption

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. Since this thread never owned the active transmission, it
abruptly aborts another thread's legitimate transfer, so return the
interruption error directly.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 0826ae8372467..b8562f8c6de04 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1007,7 +1007,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		/* wait for complete transmission of current pdu */
>  		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>  		if (err)
> -			goto err_event_drop;
> +			return err;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does resetting the state to 
ISOTP_IDLE without clearing so->cfecho expose a race condition with delayed 
local loopback echoes?

For example, if a transmission is interrupted during CAN_ISOTP_WAIT_TX_DONE, 
the wait loop is aborted and the state is reset:

net/can/isotp.c:isotp_sendmsg() {
    ...
err_event_drop:
	/* got signal: force tx state machine to be ISOTP_IDLE */
	hrtimer_cancel(&so->txfrtimer);
	hrtimer_cancel(&so->txtimer);
err_out_drop:
	/* drop this PDU and unlock a potential wait queue */
	so->tx.state = ISOTP_IDLE;
	wake_up_interruptible(&so->wait);

	return err;
}

Similarly, a TX timeout resets the state without clearing so->cfecho:

net/can/isotp.c:isotp_tx_timer_handler() {
    ...
	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 */
    ...
}

If a user starts a new transmission with a payload larger than so->tx.buflen,
the kernel sets so->tx.state to ISOTP_SENDING and allocates a new uninitialized
buffer. 

If a delayed loopback echo from the aborted transmission arrives at this time, 
could it be erroneously processed by the echo handler?

net/can/isotp.c:isotp_rcv_echo() {
    ...
	/* local echo skb with consecutive frame has been consumed */
	so->cfecho = 0;

	/* isotp_tx_timer_handler() may have raced us for so->tx.state while
	 * hrtimer_cancel() above waited for it to finish, already reporting
	 * the tx error and resetting the state; don't resume a tx job that
	 * has already been given up on.
	 */
	if (so->tx.state != ISOTP_SENDING)
		return;

	if (so->tx.idx >= so->tx.len) {
		/* we are done */
    ...
}

Because so->cfecho still matches and so->tx.state is now ISOTP_SENDING, could
this result in transmitting uninitialized kernel heap memory over the CAN bus
by continuing transmission using the old so->tx.idx and so->tx.len but the new
uninitialized so->tx.buf?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v6-0-a13c2539093d@hartkopp.net?part=5