[PATCH v2 0/2] net: can: isotp-fixes

[PATCH v2 0/2] 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.

It covers two fixes reported by Nico Yip and sashiko-bot.

Signed-off-by: Oliver Hartkopp <socketcan@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 (2):
      can: isotp: use unconditional synchronize_rcu() in isotp_release()
      can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

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

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

[PATCH v2 1/2] 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 v2 1/2] 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:
- [Critical] Race condition between `isotp_release()` and netdevice unregistration skips CAN filter unregistration, leading to a permanent memory leak and potential use-after-free.
--

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

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

This commit makes isotp_release() unconditionally call synchronize_rcu()
before cancelling timers. This closes a race where a concurrent
NETDEV_UNREGISTER could clear so->bound without waiting for in-flight
isotp_rcv() callers.

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

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between
isotp_release() and netdevice unregistration that can skip CAN filter
unregistration?

If unregister_netdevice_many() unlists the netdevice from net->dev_by_index
using unlist_netdevice(), and isotp_release() runs immediately after this but
before the NETDEV_UNREGISTER notifier is dispatched:

isotp_release() {
    ...
    list_del(&so->notifier);
    ...
    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {
        ...
    }
}

Since the device is unlisted, this returns NULL, causing isotp_release() to
silently skip unregistering the CAN RX filters.

When the NETDEV_UNREGISTER notifier eventually runs in isotp_notifier(), it
misses the socket because it was already unlinked from isotp_notifier_list.

Could this permanently leak the CAN filters with a dangling pointer to the
freed socket, leading to a use-after-free? Can this be triggered by an
unprivileged user in a user namespace with CAP_NET_ADMIN by repeatedly
creating and destroying vcan interfaces while simultaneously creating and
closing ISOTP sockets?

> -				synchronize_rcu();
>  			}
>  		}
>  	}

[ ... ]

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

[PATCH v2 2/2] 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_notify()'s NETDEV_UNREGISTER path left the tx/rx state machine
and hrtimers running after clearing so->bound, letting a stale timer
leak leftover data if the socket got rebound; both are now reset
under synchronize_net(), and isotp_sendmsg() re-validates so->bound.

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

diff --git a/net/can/isotp.c b/net/can/isotp.c
index d30937345bcd..d076226ac713 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,10 +1317,11 @@ 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;
@@ -1327,18 +1337,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 +1360,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 +1581,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 +1597,34 @@ 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;
+
+		/* wait for in-flight isotp_rcv()/isotp_rcv_echo() calls to
+		 * finish so they can't leak stale tx data via a re-armed
+		 * timer after a rebind.
+		 * synchronize_net() is expedited here since rtnl_lock is held
+		 */
+		synchronize_net();
+
+		hrtimer_cancel(&so->txfrtimer);
+		hrtimer_cancel(&so->txtimer);
+		hrtimer_cancel(&so->rxtimer);
+
+		/* reset tx/rx state so a following bind() starts clean */
+		so->tx.state = ISOTP_IDLE;
+		so->rx.state = ISOTP_IDLE;
+		so->cfecho = 0;
+		wake_up_interruptible(&so->wait);
+
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1643,10 +1680,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 v2 2/2] 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:
- [Critical] Forcibly resetting `so->tx.state = ISOTP_IDLE` in `isotp_notify()` breaks the lockless mutual exclusion mechanism used by `isotp_sendmsg()`, leading to concurrent execution, memory corruption, and an Out-of-Bounds (OOB) write.
--

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

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

This patch fixes a use-after-free race condition by holding a tracked
reference to the bound net_device in the socket from bind() onward. It
serializes bind()/release() with rtnl_lock() and resets the tx/rx state
machine and hrtimers in isotp_notify() during NETDEV_UNREGISTER.

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..d076226ac713f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1582,10 +1599,30 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
>  			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;
> +
> +		/* wait for in-flight isotp_rcv()/isotp_rcv_echo() calls to
> +		 * finish so they can't leak stale tx data via a re-armed
> +		 * timer after a rebind.
> +		 * synchronize_net() is expedited here since rtnl_lock is held
> +		 */
> +		synchronize_net();
> +
> +		hrtimer_cancel(&so->txfrtimer);
> +		hrtimer_cancel(&so->txtimer);
> +		hrtimer_cancel(&so->rxtimer);
> +
> +		/* reset tx/rx state so a following bind() starts clean */
> +		so->tx.state = ISOTP_IDLE;
> +		so->rx.state = ISOTP_IDLE;

[Severity: Critical]
Does forcibly resetting so->tx.state to ISOTP_IDLE here break the lockless
mutual exclusion mechanism used by isotp_sendmsg()?

isotp_sendmsg() relies on cmpxchg() to protect the state machine without
holding lock_sock():

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

If a thread is actively sleeping inside isotp_sendmsg() (for instance,
waiting for buffer space in sock_alloc_send_skb()), a concurrent
NETDEV_UNREGISTER will invoke isotp_notify() which clears the state here.

Because the state is externally reset, a subsequent bind() and sendmsg()
could successfully claim ISOTP_SENDING again, allowing two threads to execute
isotp_sendmsg() concurrently on the same socket.

Could this concurrent execution corrupt the shared so->tx state machine,
allowing so->tx.idx to exceed so->tx.len?

If so, later in isotp_send_cframe(), the space calculation would become
negative:

		space = so->tx.len - so->tx.idx;
		for (i = space; i < so->ll.tx_dl - SF_PCI_SZ4 - ae - off; i++)
			cf->data[SF_PCI_SZ4 + ae + off + i] = so->opt.txpad_content;

Can this cause a backward out-of-bounds write into the SKB headroom?

> +		so->cfecho = 0;
> +		wake_up_interruptible(&so->wait);
> +
>  		release_sock(sk);
>  
>  		sk->sk_err = ENODEV;

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