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

[PATCH v8 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.

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

Complete concept change than former fix approach by using so->rx_lock
for the cfecho and in sendmsg to properly shutdown and drain the former
(maybe faulty) PDU transmission before starting a new sending attempt.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v8:
- patch 3: properly terminate the former transmission before starting
- patch 4: formerly patch 5 as the former patch 4 is squashed into patch 3
- Link to v7: https://patch.msgid.link/20260711-isotp-fixes-v7-0-f03af24d8397@hartkopp.net

Changes in v7:
- patch 3: add helper so->tx_gen to monitor new ISOTP_SENDING attempts
- patch 4: prevent missasignment of sk_err report with new so->tx_gen
- Link to v6: https://patch.msgid.link/20260710-isotp-fixes-v6-0-a13c2539093d@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 (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/TX timers and frame reception
      can: isotp: fix state machine corruption on signal interruption

 net/can/isotp.c | 271 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 206 insertions(+), 65 deletions(-)
---
base-commit: dd3210c47e8d3ac6b4e9141fc68acc03b38c0ba3
change-id: 20260710-isotp-fixes-ead7480d67bf

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

[PATCH v8 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 v8 1/4] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Race condition between isotp_sendmsg() error paths and isotp_rcv_fc() allows leaking uninitialized kernel heap memory over the CAN bus.
- [High] CAN filter leak due to a race condition between socket release and netdevice unregistration.
- [High] State Machine Leak and Cross-Interface Data Leakage on NETDEV_UNREGISTER.
- [High] Lockless updates to the TX state machine in isotp_sendmsg() race with concurrent CAN frame reception.
- [Medium] Missing Error Handling in isotp_bind() ignores can_rx_register() failures.
--

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

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

isotp_notify() unregisters the (RCU) CAN filters and clears so->bound
without waiting for a grace period. This commit updates isotp_release() to
unconditionally call synchronize_rcu() before cancelling timers, fixing a
race condition where NETDEV_UNREGISTER could lead to a use-after-free timer
callback on the freed socket.

Link: 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);
> -				synchronize_rcu();
>  			}
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does a race condition exist between socket
release and netdevice unregistration that can leak filters?

If isotp_release() runs during the window where unregister_netdevice_many()
has unlisted the device from the IDR but before notifiers are invoked:

net/can/isotp.c:isotp_release() {
    ...
    list_del(&so->notifier);
    ...
    dev = dev_get_by_index(net, so->ifindex);
    ...
}

Because the socket is unlinked from the notifier list, the notifier won't
clean it up. However, dev_get_by_index() returns NULL since the device is
unlisted, causing isotp_release() to silently skip calling
can_rx_unregister() and permanently leak the receiver memory.

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

[Severity: High]
This isn't a bug introduced by this patch, but does the NETDEV_UNREGISTER
handler fail to stop active state machines?

When the unregister event is handled:

net/can/isotp.c:isotp_notify() {
    ...
    case NETDEV_UNREGISTER:
        lock_sock(sk);
        /* remove current filters & unregister */
    ...
}

The code clears so->bound and so->ifindex but omits explicitly aborting
active state machines (so->tx.state) or cancelling tx timers. If a thread
is blocked in isotp_sendmsg(), it remains hung until a timer expires.

Additionally, if userspace calls bind() on a new interface, the state isn't
reset, and when the active timer fires, it could transmit remaining frames
of the old PDU out of the newly bound network interface.

[Severity: Medium]
This is a pre-existing issue, but does isotp_bind() ignore filter
registration failures?

net/can/isotp.c:isotp_bind() {
    ...
    if (isotp_register_rxid(so))
        can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
                        isotp_rcv, sk, "isotp", sk);
    ...
}

If can_rx_register() fails with -ENOMEM, isotp_bind() ignores the return
value, sets so->bound = 1, and returns 0. The socket then silently fails to
receive or echo frames, and later isotp_release() will trigger a warning
splat when attempting to unregister the missing filter.

[Severity: High]
This isn't a bug introduced by this patch, but is there a race with
concurrent CAN frame reception when updating the state locklessly?

net/can/isotp.c:isotp_sendmsg() {
    ...
    so->tx.state = ISOTP_WAIT_FIRST_FC;
    ...
}

If a spoofed or leftover Flow Control frame is received concurrently on
another CPU:

net/can/isotp.c:isotp_rcv_fc() {
    ...
    if (so->tx.state != ISOTP_WAIT_FC &&
        so->tx.state != ISOTP_WAIT_FIRST_FC)
    ...
}

The softirq path can see ISOTP_WAIT_FIRST_FC and immediately transition to
ISOTP_SENDING, transmitting a Consecutive Frame before isotp_sendmsg() has
sent the First Frame, scrambling the protocol state.

[Severity: Critical]
This is a pre-existing issue, but can the signal error path in
isotp_sendmsg() leak uninitialized kernel memory?

If isotp_sendmsg() is interrupted by a signal, it sets the state to idle:

net/can/isotp.c:isotp_sendmsg() {
    ...
    so->tx.state = ISOTP_IDLE;
    hrtimer_cancel(&so->txfrtimer);
    ...
}

If a concurrent isotp_rcv_fc() already passed the state check, and
userspace immediately calls sendmsg() again with a large size:

net/can/isotp.c:isotp_sendmsg() {
    ...
    u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
    ...
}

The delayed isotp_rcv_fc() could resume, transition the state, and transmit
the newly allocated, uninitialized so->tx.buf over the CAN bus before the
data has been copied from userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=1

[PATCH v8 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|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 v8 2/4] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

[sashiko-bot]

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

Pre-existing issues:
- [Critical] A TOCTOU race condition in `isotp_sendmsg` allows concurrent modification of socket options, leading to an SKB heap overflow and `skb_over_panic`.
- [High] The check in `isotp_bind()` to prevent rebinding during an active state machine does not fully prevent cross-interface packet injection from RCU-delayed softirqs.
--

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

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

This tracks the bound net_device with a reference instead of re-resolving
it from ifindex during isotp_release(). It also rejects isotp_bind() with
-EAGAIN if the state machines aren't idle yet, avoiding timers acting on a
newly bound interface.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index d30937345bcdc..44c044eb83e12 100644
> --- 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;
>  	}
>  
> +	/* 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;
> +	}

[Severity: Critical]
This is a pre-existing issue, but does a time-of-check to time-of-use race
still exist here if sock_alloc_send_skb() blocks later in isotp_sendmsg()?

If sock_alloc_send_skb(sk, so->ll.mtu) blocks during allocation, a concurrent
NETDEV_UNREGISTER could trigger isotp_notify(), which acquires lock_sock()
and sets so->bound = 0.

Userspace could then call setsockopt(CAN_ISOTP_LL_OPTS), which passes its
!so->bound check and modifies so->ll.mtu and so->tx.ll_dl.

When isotp_sendmsg() resumes, it has a smaller SKB but could use the new
larger MTU, or isotp_fill_dataframe() could use the new so->tx.ll_dl to copy
too many bytes into the small SKB:

net/can/isotp.c:isotp_fill_dataframe() {
    ...
    for (i = 0; i < num; i++)
        cf->data[pcilen + i] = so->tx.buf[so->tx.idx++];
    ...
}

Could this lead to a heap out-of-bounds write overwriting skb_shared_info?

[ ... ]

> @@ -1317,6 +1327,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		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;
> +	}

[Severity: High]
This is a pre-existing issue, but does this state machine check fully prevent
cross-interface packet injection from RCU-delayed softirqs?

If a packet arrives on an old device and is queued via can_rx_register()'s
softirq, and concurrently the device is unregistered (clearing so->bound),
userspace could call bind() for a new device.

Since isotp_rcv() hasn't acquired so->rx_lock to change the state yet, the
lockless check here passes and so->dev is updated. Could the delayed softirq
then acquire so->rx_lock, transition the state, and send a Flow Control frame
using the newly bound so->ifindex?

Would isotp_rcv() need to verify that skb->dev matches the currently bound
so->dev after acquiring so->rx_lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=2

[PATCH v8 3/4] 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 is already running on another CPU,
hrtimer_cancel() waits for it, and by the time it returns, the timer
handler may have already reported a timeout and reset the state
machine.

so->rx_lock now also covers the TX side: isotp_rcv_echo() takes it
(isotp_rcv_fc()/isotp_rcv_cf() already run under it via isotp_rcv()),
and sendmsg() takes it briefly when claiming a new transfer to cancel
all three TX timers and clear so->cfecho itself. A concurrent claim
cannot get past this point while an old-generation frame is still
being processed under the same lock, so isotp_rcv_fc()/isotp_rcv_cf()
only need a plain state recheck after hrtimer_cancel().

Two spots additionally need so->tx_gen, because sendmsg()'s initial
claim is a lock-free cmpxchg() to ISOTP_SENDING that can succeed while
the old generation's rx_lock holder is still active:

 - isotp_rcv_echo() checks state == ISOTP_SENDING, exactly what a
   brand new claim also sets before it ever reaches the lock; so->tx_gen
   tells the two apart.
 - sendmsg()'s error paths unconditionally wrote so->tx.state =
   ISOTP_IDLE; that write is now gated on so->tx_gen too, since a new
   claim can succeed while we're still cancelling our own timers.

The use of rx_lock guarantees a new transfer can't have armed anything
of its own yet while we hold it.

so->tx_gen is otherwise only used by isotp_tx_timer_handler()/
isotp_echo_timer_handler(), which must stay lock-free to avoid
deadlocking against the hrtimer_cancel() calls made under so->rx_lock
elsewhere.

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 | 173 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 138 insertions(+), 35 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 44c044eb83e1..c5b44d298618 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -156,18 +156,19 @@ struct isotp_sock {
 	netdevice_tracker dev_tracker;
 	canid_t txid;
 	canid_t rxid;
 	ktime_t tx_gap;
 	ktime_t lastrxcf_tstamp;
-	struct hrtimer rxtimer, txtimer, txfrtimer;
+	struct hrtimer rxtimer, txtimer, txfrtimer, echotimer;
 	struct can_isotp_options opt;
 	struct can_isotp_fc_options rxfc, txfc;
 	struct can_isotp_ll_options ll;
 	u32 frame_txtime;
 	u32 force_tx_stmin;
 	u32 force_rx_stmin;
 	u32 cfecho; /* consecutive frame echo tag */
+	u32 tx_gen; /* generation, bumped per new tx transfer */
 	struct tpcon rx, tx;
 	struct list_head notifier;
 	wait_queue_head_t wait;
 	spinlock_t rx_lock; /* protect single thread state machine */
 };
@@ -376,10 +377,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_timeout() may have given up on this job while
+	 * hrtimer_cancel() above waited for it to finish; so->rx_lock
+	 * (held by our caller isotp_rcv()) rules out a concurrent claim,
+	 * so a plain recheck is enough here.
+	 */
+	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;
@@ -422,11 +432,11 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
 	switch (cf->data[ae] & 0x0F) {
 	case ISOTP_FC_CTS:
 		so->tx.bs = 0;
 		so->tx.state = ISOTP_SENDING;
 		/* send CF frame and enable echo timeout handling */
-		hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
+		hrtimer_start(&so->echotimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
 			      HRTIMER_MODE_REL_SOFT);
 		isotp_send_cframe(so);
 		break;
 
 	case ISOTP_FC_WT:
@@ -575,10 +585,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 */
@@ -868,80 +886,130 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
 static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 {
 	struct sock *sk = (struct sock *)data;
 	struct isotp_sock *so = isotp_sk(sk);
 	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+	u32 gen;
 
 	/* only handle my own local echo CF/SF skb's (no FF!) */
-	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
+	if (skb->sk != sk)
 		return;
 
+	/* unlike isotp_rcv_fc()/isotp_rcv_cf(), not already under so->rx_lock
+	 * (no isotp_rcv() caller here), so take it ourselves
+	 */
+	spin_lock(&so->rx_lock);
+
+	/* so->cfecho may since belong to a new transfer; recheck under lock */
+	if (so->cfecho != *(u32 *)cf->data)
+		goto out_unlock;
+
+	gen = READ_ONCE(so->tx_gen);
+
 	/* cancel local echo timeout */
-	hrtimer_cancel(&so->txtimer);
+	hrtimer_cancel(&so->echotimer);
 
 	/* local echo skb with consecutive frame has been consumed */
 	so->cfecho = 0;
 
+	/* so->rx_lock stops a new claim from arming a timer of its own
+	 * while we're still here, but not from the lock-free cmpxchg()
+	 * claim itself: state alone can't tell our SENDING from a new
+	 * transfer's, so also gate on gen.
+	 */
+	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
+		goto out_unlock;
+
 	if (so->tx.idx >= so->tx.len) {
 		/* we are done */
 		so->tx.state = ISOTP_IDLE;
 		wake_up_interruptible(&so->wait);
-		return;
+		goto out_unlock;
 	}
 
 	if (so->txfc.bs && so->tx.bs >= so->txfc.bs) {
 		/* stop and wait for FC with timeout */
 		so->tx.state = ISOTP_WAIT_FC;
 		hrtimer_start(&so->txtimer, ktime_set(ISOTP_FC_TIMEOUT, 0),
 			      HRTIMER_MODE_REL_SOFT);
-		return;
+		goto out_unlock;
 	}
 
 	/* no gap between data frames needed => use burst mode */
 	if (!so->tx_gap) {
 		/* enable echo timeout handling */
-		hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
+		hrtimer_start(&so->echotimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
 			      HRTIMER_MODE_REL_SOFT);
 		isotp_send_cframe(so);
-		return;
+		goto out_unlock;
 	}
 
 	/* start timer to send next consecutive frame with correct delay */
 	hrtimer_start(&so->txfrtimer, so->tx_gap, HRTIMER_MODE_REL_SOFT);
+
+out_unlock:
+	spin_unlock(&so->rx_lock);
 }
 
-static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
+/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get
+ * cancelled under so->rx_lock elsewhere, so this must stay lock-free to
+ * avoid deadlocking with that; uses so->tx_gen instead to avoid tainting
+ * a new transfer with an error from the one that just timed out.
+ */
+static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
 {
-	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
-					     txtimer);
 	struct sock *sk = &so->sk;
+	u32 gen = READ_ONCE(so->tx_gen);
+	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 the timeout if the state is still unchanged */
+	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);
+	if (READ_ONCE(so->tx_gen) == gen) {
+		/* 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;
 }
 
+/* so->txtimer: fires when a Flow Control frame does not arrive in time */
+static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
+{
+	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
+					     txtimer);
+
+	return isotp_tx_timeout(so);
+}
+
+/* so->echotimer: fires when a sent CF/SF's local echo does not arrive */
+static enum hrtimer_restart isotp_echo_timer_handler(struct hrtimer *hrtimer)
+{
+	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
+					     echotimer);
+
+	return isotp_tx_timeout(so);
+}
+
 static enum hrtimer_restart isotp_txfr_timer_handler(struct hrtimer *hrtimer)
 {
 	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
 					     txfrtimer);
 
 	/* start echo timeout handling and cover below protocol error */
-	hrtimer_start(&so->txtimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
+	hrtimer_start(&so->echotimer, ktime_set(ISOTP_ECHO_TIMEOUT, 0),
 		      HRTIMER_MODE_REL_SOFT);
 
 	/* cfecho should be consumed by isotp_rcv_echo() here */
 	if (so->tx.state == ISOTP_SENDING && !so->cfecho)
 		isotp_send_cframe(so);
@@ -958,12 +1026,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	struct net_device *dev;
 	struct canfd_frame *cf;
 	int ae = (so->opt.flags & CAN_ISOTP_EXTEND_ADDR) ? 1 : 0;
 	int wait_tx_done = (so->opt.flags & CAN_ISOTP_WAIT_TX_DONE) ? 1 : 0;
 	s64 hrtimer_sec = ISOTP_ECHO_TIMEOUT;
+	struct hrtimer *tx_hrt = &so->echotimer;
 	int off;
 	int err;
+	u32 gen;
 
 	if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
 		return -EADDRNOTAVAIL;
 
 	while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
@@ -978,10 +1048,26 @@ 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;
 	}
 
+	/* new transfer: bump the generation (gen is used below and by
+	 * isotp_tx_timeout()/isotp_rcv_echo()) and drain the old one -
+	 * cancel its timers ourselves in case one is still concurrently
+	 * completing it, and take so->rx_lock so no isotp_rcv_fc()/
+	 * isotp_rcv_echo() call for it is still in flight.
+	 */
+	gen = READ_ONCE(so->tx_gen) + 1;
+	WRITE_ONCE(so->tx_gen, gen);
+
+	spin_lock_bh(&so->rx_lock);
+	hrtimer_cancel(&so->txtimer);
+	hrtimer_cancel(&so->echotimer);
+	hrtimer_cancel(&so->txfrtimer);
+	so->cfecho = 0;
+	spin_unlock_bh(&so->rx_lock);
+
 	/* 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;
@@ -1098,17 +1184,18 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			/* standard flow control check */
 			so->tx.state = ISOTP_WAIT_FIRST_FC;
 
 			/* start timeout for FC */
 			hrtimer_sec = ISOTP_FC_TIMEOUT;
+			tx_hrt = &so->txtimer;
 
 			/* no CF echo tag for isotp_rcv_echo() (FF-mode) */
 			so->cfecho = 0;
 		}
 	}
 
-	hrtimer_start(&so->txtimer, ktime_set(hrtimer_sec, 0),
+	hrtimer_start(tx_hrt, ktime_set(hrtimer_sec, 0),
 		      HRTIMER_MODE_REL_SOFT);
 
 	/* send the first or only CAN frame */
 	cf->flags = so->ll.tx_flags;
 
@@ -1118,17 +1205,14 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	dev_put(dev);
 	if (err) {
 		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
 			       __func__, ERR_PTR(err));
 
+		spin_lock_bh(&so->rx_lock);
 		/* no transmission -> no timeout monitoring */
-		hrtimer_cancel(&so->txtimer);
-
-		/* reset consecutive frame echo tag */
-		so->cfecho = 0;
-
-		goto err_out_drop;
+		hrtimer_cancel(tx_hrt);
+		goto err_out_drop_locked;
 	}
 
 	if (wait_tx_done) {
 		/* wait for complete transmission of current pdu */
 		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
@@ -1140,18 +1224,32 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			return err;
 	}
 
 	return size;
 
+err_out_drop:
+	spin_lock_bh(&so->rx_lock);
+	goto err_out_drop_locked;
 err_event_drop:
-	/* got signal: force tx state machine to be idle */
-	so->tx.state = ISOTP_IDLE;
+	/* got signal: cancel timers under so->rx_lock, held through
+	 * err_out_drop_locked below, so a concurrent claim's own drain
+	 * step can't proceed until we're done
+	 */
+	spin_lock_bh(&so->rx_lock);
 	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;
+	hrtimer_cancel(&so->echotimer);
+err_out_drop_locked:
+	/* so->rx_lock stops a new claim from cancelling anything of its own
+	 * while we're still here, but not from the lock-free cmpxchg()
+	 * claim itself; only drop our own PDU if that hasn't happened
+	 */
+	if (READ_ONCE(so->tx_gen) == gen) {
+		so->cfecho = 0;
+		so->tx.state = ISOTP_IDLE;
+	}
+	spin_unlock_bh(&so->rx_lock);
 	wake_up_interruptible(&so->wait);
 
 	return err;
 }
 
@@ -1261,10 +1359,11 @@ static int isotp_release(struct socket *sock)
 	 */
 	synchronize_rcu();
 
 	hrtimer_cancel(&so->txfrtimer);
 	hrtimer_cancel(&so->txtimer);
+	hrtimer_cancel(&so->echotimer);
 	hrtimer_cancel(&so->rxtimer);
 
 	sock_orphan(sk);
 	sock->sk = NULL;
 
@@ -1700,14 +1799,18 @@ static int isotp_init(struct sock *sk)
 	so->rx.buf = so->rx.sbuf;
 	so->tx.buf = so->tx.sbuf;
 	so->rx.buflen = ARRAY_SIZE(so->rx.sbuf);
 	so->tx.buflen = ARRAY_SIZE(so->tx.sbuf);
 
-	hrtimer_setup(&so->rxtimer, isotp_rx_timer_handler, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
-	hrtimer_setup(&so->txtimer, isotp_tx_timer_handler, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
-	hrtimer_setup(&so->txfrtimer, isotp_txfr_timer_handler, CLOCK_MONOTONIC,
-		      HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&so->rxtimer, isotp_rx_timer_handler,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&so->txtimer, isotp_tx_timer_handler,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&so->echotimer, isotp_echo_timer_handler,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+	hrtimer_setup(&so->txfrtimer, isotp_txfr_timer_handler,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 
 	init_waitqueue_head(&so->wait);
 	spin_lock_init(&so->rx_lock);
 
 	spin_lock(&isotp_notifier_lock);

-- 
2.53.0

Re: [PATCH v8 3/4] 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] Error paths in `isotp_sendmsg()` unconditionally cancel hrtimers, which can inadvertently destroy the active timers of a newly started concurrent transfer.
- [High] A delayed local echo frame from a previous transfer can bypass the `tx_gen` validation and corrupt a newly started transfer due to a TOCTOU race with `so->cfecho`.
- [High] The timeout handler incorrectly suppresses the socket timeout error (`ECOMM`) if a concurrent transfer claims the socket, causing the timed-out `sendmsg()` to silently drop data and falsely report success.

Pre-existing issues:
- [High] Concurrent `sendmsg` calls cause the active sender to hang waiting for a new sender to finish, leading to hangs or cross-transfer status leaks.
--

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

This commit introduces so->rx_lock to protect the TX state machine against
concurrent hrtimer execution and frame reception. It also adds a tx_gen
generation counter to safely distinguish between old and new TX transfers
when cancelling timers or processing echo frames locklessly.

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..c5b44d298618b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -870,22 +888,42 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	struct sock *sk = (struct sock *)data;
>  	struct isotp_sock *so = isotp_sk(sk);
>  	struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +	u32 gen;
>  
>  	/* only handle my own local echo CF/SF skb's (no FF!) */
> -	if (skb->sk != sk || so->cfecho != *(u32 *)cf->data)
> +	if (skb->sk != sk)
>  		return;
>  
> +	/* unlike isotp_rcv_fc()/isotp_rcv_cf(), not already under so->rx_lock
> +	 * (no isotp_rcv() caller here), so take it ourselves
> +	 */
> +	spin_lock(&so->rx_lock);
> +
> +	/* so->cfecho may since belong to a new transfer; recheck under lock */
> +	if (so->cfecho != *(u32 *)cf->data)
> +		goto out_unlock;
> +
> +	gen = READ_ONCE(so->tx_gen);

[Severity: High]
Can a delayed echo frame from a previous transfer bypass the validation and
corrupt a newly started transfer here?

If transfer 1 times out, it leaves its echo tag in so->cfecho.
If transfer 2 then claims the socket, it locklessly increments so->tx_gen
in isotp_sendmsg() before acquiring so->rx_lock to clear so->cfecho.

If a delayed echo for transfer 1 arrives in that small window, isotp_rcv_echo()
will acquire so->rx_lock, match the old so->cfecho (which hasn't been cleared
yet), and read the new so->tx_gen.

Does this allow the stale echo to pass the generation check below and disrupt
the protocol state machine of the new transfer?

>  
>  	/* cancel local echo timeout */
> -	hrtimer_cancel(&so->txtimer);
> +	hrtimer_cancel(&so->echotimer);
>  
>  	/* local echo skb with consecutive frame has been consumed */
>  	so->cfecho = 0;
>  
> +	/* so->rx_lock stops a new claim from arming a timer of its own
> +	 * while we're still here, but not from the lock-free cmpxchg()
> +	 * claim itself: state alone can't tell our SENDING from a new
> +	 * transfer's, so also gate on gen.
> +	 */
> +	if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
> +		goto out_unlock;
[ ... ]
> -static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
> +/* shared by so->txtimer's and so->echotimer's callbacks. Both timers get
> + * cancelled under so->rx_lock elsewhere, so this must stay lock-free to
> + * avoid deadlocking with that; uses so->tx_gen instead to avoid tainting
> + * a new transfer with an error from the one that just timed out.
> + */
> +static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  {
> -	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
> -					     txtimer);
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
> +	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 the timeout if the state is still unchanged */
> +	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);
> +	if (READ_ONCE(so->tx_gen) == gen) {
> +		/* report 'communication error on send' */
> +		sk->sk_err = ECOMM;
> +		if (!sock_flag(sk, SOCK_DEAD))
> +			sk_error_report(sk);
> +	}

[Severity: High]
Does this suppress the socket timeout error if a concurrent transfer claims
the socket right after the state transitions to ISOTP_IDLE?

Once cmpxchg() sets so->tx.state to ISOTP_IDLE, a concurrent thread calling
isotp_sendmsg() can immediately claim the state and increment so->tx_gen.

If that happens before the READ_ONCE(so->tx_gen) == gen check, the error
reporting is skipped. Does this cause the original timed-out isotp_sendmsg()
call to falsely report success to userspace instead of returning an error?

>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;
>  }
[ ... ]
> @@ -1120,13 +1207,10 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
>  			       __func__, ERR_PTR(err));
>  
> +		spin_lock_bh(&so->rx_lock);
>  		/* no transmission -> no timeout monitoring */
> -		hrtimer_cancel(&so->txtimer);
> -
> -		/* reset consecutive frame echo tag */
> -		so->cfecho = 0;
> -
> -		goto err_out_drop;
> +		hrtimer_cancel(tx_hrt);

[Severity: High]
Can this unconditionally cancel the timer of a newly started concurrent
transfer?

If can_send() fails, this path cancels tx_hrt before verifying ownership
via tx_gen. If a concurrent sendmsg() has already claimed the socket and
started its own timers, will this cancel the new transfer's timers and
cause it to hang indefinitely without timeout monitoring?

> +		goto err_out_drop_locked;
>  	}
>  
>  	if (wait_tx_done) {
> @@ -1142,14 +1226,28 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  
>  	return size;

[Severity: High]
This isn't a bug introduced by this patch, but does the wait condition here
handle concurrent senders correctly?

When using CAN_ISOTP_WAIT_TX_DONE, isotp_sendmsg() waits on:
    wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);

If transfer 1's transmission completes (setting ISOTP_IDLE and waking waiters),
but transfer 2 concurrently claims ISOTP_SENDING before transfer 1 wakes up,
transfer 1 will evaluate so->tx.state == ISOTP_IDLE as false.

Will transfer 1 then go back to sleep, incorrectly hanging while waiting on
transfer 2's completion?

>  
> +err_out_drop:
> +	spin_lock_bh(&so->rx_lock);
> +	goto err_out_drop_locked;
>  err_event_drop:
> -	/* got signal: force tx state machine to be idle */
> -	so->tx.state = ISOTP_IDLE;
> +	/* got signal: cancel timers under so->rx_lock, held through
> +	 * err_out_drop_locked below, so a concurrent claim's own drain
> +	 * step can't proceed until we're done
> +	 */
> +	spin_lock_bh(&so->rx_lock);
>  	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;
> +	hrtimer_cancel(&so->echotimer);

[Severity: High]
Similar to the can_send() error path above, this unconditionally cancels
all TX timers without checking if the socket is still owned by the current
transfer's generation.

If a timeout already occurred and a new transfer is active, will cancelling
these timers destroy the active timers of the new transfer?

> +err_out_drop_locked:
> +	/* so->rx_lock stops a new claim from cancelling anything of its own
> +	 * while we're still here, but not from the lock-free cmpxchg()
> +	 * claim itself; only drop our own PDU if that hasn't happened
> +	 */
> +	if (READ_ONCE(so->tx_gen) == gen) {
> +		so->cfecho = 0;
> +		so->tx.state = ISOTP_IDLE;
> +	}
> +	spin_unlock_bh(&so->rx_lock);
>  	wake_up_interruptible(&so->wait);
>  
>  	return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=3

[PATCH v8 4/4] 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 c5b44d298618..ceb590ae058e 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1045,11 +1045,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;
 	}
 
 	/* new transfer: bump the generation (gen is used below and by
 	 * isotp_tx_timeout()/isotp_rcv_echo()) and drain the old one -
 	 * cancel its timers ourselves in case one is still concurrently

-- 
2.53.0

Re: [PATCH v8 4/4] can: isotp: fix state machine corruption on signal interruption

[sashiko-bot]

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

Pre-existing issues:
- [High] The `so->tx_gen` mechanism fails to protect against delayed local echoes from aborted transfers if the CAN payload matches, leading to protocol pacing violations and timeouts.
- [High] Signal interruption during `wait_tx_done` causes racy state clobbering of concurrent transfers due to a stolen wakeup and lockless state update.
--

--- Patch [4]: [PATCH v8 4/4] can: isotp: fix state machine corruption on signal interruption ---
commit 7dee50f317c5e02a3ea6610e45f4dde065c0b96c
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix state machine corruption on signal interruption

This commit fixes a bug where a signal interrupting a thread waiting to start
a new transfer incorrectly aborts an ongoing transfer. By returning the error
directly, it avoids executing the cleanup path that forces the state back to
ISOTP_IDLE.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c5b44d298618b..ceb590ae058eb 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1047,7 +1047,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 is a pre-existing issue, but does a similar race exist later in
isotp_sendmsg() when handling wait_tx_done?

If Thread A waits in wait_tx_done with a signal pending, and concurrently
the transfer finishes (setting state to ISOTP_IDLE and waking waiters),
Thread B could wake up and claim the socket locklessly:

Thread B:
    while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {

Before Thread B increments so->tx_gen, if Thread A wakes up, evaluates its
wait condition, returns -ERESTARTSYS, and jumps to err_event_drop:

Thread A:
    if (wait_tx_done) {
        err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
        if (err)
            goto err_event_drop;
    }

Would Thread A then incorrectly reset the state back to ISOTP_IDLE while
Thread B is actively using the socket?

Thread A in err_event_drop:
    if (READ_ONCE(so->tx_gen) == gen) {
        so->cfecho = 0;
        so->tx.state = ISOTP_IDLE;
    }

>  	}
>  
>  	/* new transfer: bump the generation (gen is used below and by

[Severity: High]
This isn't a bug introduced by this patch, but does the so->tx_gen mechanism
fully protect against delayed local echoes from aborted transfers?

If a transfer is aborted while its CAN frame is already queued, and a new
transfer begins with matching first 4 bytes of payload, could the delayed
local echo from the aborted frame arrive and be falsely validated?

In isotp_rcv_echo():

    if (so->cfecho != *(u32 *)cf->data)
        goto out_unlock;
    
    gen = READ_ONCE(so->tx_gen);
    ...
    if (so->tx.state != ISOTP_SENDING || READ_ONCE(so->tx_gen) != gen)
        goto out_unlock;

Since gen is read directly from the current socket state (so->tx_gen) rather
than being persistently attached to the SKB, does this allow a stale echo to
prematurely advance the new transfer's state machine, leading to pacing
(STmin) violations and timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-isotp-fixes-v8-0-c39220a4e442@hartkopp.net?part=4