[PATCH] can: isotp: hold a reference to the bound netdevice

[PATCH] can: isotp: hold a reference to the bound netdevice

[Vasileios Almpanis]

isotp_release() looked up the bound netdevice with
dev_get_by_index(so->ifindex) and only unregistered the socket's CAN
receivers if that lookup succeeded. This is unreliable while the device
is going away: unregister_netdevice_many() removes the device from the
ifindex hash before the NETDEV_UNREGISTER notifier is fired, with a
synchronize_net() in between.

A close() that runs in that window first removes the socket from
isotp_notifier_list, so the pending notifier no longer sees it, and then
gets NULL back from dev_get_by_index() and skips the unregister. The two
struct receiver entries that were allocated by can_rx_register() in
isotp_bind() (the rx filter and the tx echo filter) are then never freed
and leak once the device is destroyed.

Fix it the same way can-raw already does: keep a tracked reference to the
bound device in so->dev and unregister through it, and take rtnl_lock()
in isotp_bind() and isotp_release() to serialise against the
NETDEV_UNREGISTER notifier. The dev_get_by_index() lookup in the release
path is dropped.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: stable@vger.kernel.org
Reported-by: syzbot+24201717ed2da31b8fae@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?id=58676a0f698531996a42612c552e894a55b9732b
Signed-off-by: Vasileios Almpanis <vasilisalmpanis@gmail.com>
---
 net/can/isotp.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index c48b4a818297..308d18040b6d 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -69,6 +69,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
 #include <linux/slab.h>
+#include <linux/rtnetlink.h>
 #include <net/can.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>
@@ -152,6 +153,8 @@ 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;
@@ -1219,27 +1222,22 @@ static int isotp_release(struct socket *sock)
 	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);
-				synchronize_rcu();
-			}
-		}
+	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->dev = NULL;
+		synchronize_rcu();
 	}
 
 	hrtimer_cancel(&so->txfrtimer);
@@ -1253,6 +1251,7 @@ static int isotp_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
+	rtnl_unlock();
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
@@ -1303,6 +1302,7 @@ 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) {
@@ -1347,16 +1347,20 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	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;
 
+	so->dev = dev;
+	netdev_hold(dev, &so->dev_tracker, GFP_KERNEL);
+
+	dev_put(dev);
+
 out:
 	release_sock(sk);
+	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -1559,7 +1563,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->ifindex != dev->ifindex)
+	if (so->dev != dev)
 		return;
 
 	switch (msg) {
@@ -1575,10 +1579,12 @@ 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(dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
+		so->dev = NULL;
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
-- 
2.47.3

Re: [PATCH] can: isotp: hold a reference to the bound netdevice

[sashiko-bot]

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

New issues:
- [High] Calling synchronize_rcu() while holding rtnl_lock() introduces severe latency spikes and potential denial of service system-wide.

Pre-existing issues:
- [Critical] Skipping synchronize_rcu() in isotp_release() when the socket is unbound leads to a use-after-free of the socket structure by re-armed hrtimers.
--

commit da93bcd8877d85ffe0b043e3045c3374b10e38e5
Author: Vasileios Almpanis <vasilisalmpanis@gmail.com>

can: isotp: hold a reference to the bound netdevice
    
isotp_release() looked up the bound netdevice with
dev_get_by_index(so->ifindex) and only unregistered the socket's CAN
receivers if that lookup succeeded. This is unreliable while the device
is going away.

Cc: stable@vger.kernel.org
Closes: https://syzkaller.appspot.com/bug?id=58676a0f698531996a42612c552e894a55b9732b

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..308d18040b6db 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1219,27 +1222,22 @@ static int isotp_release(struct socket *sock)
>  	list_del(&so->notifier);
>  	spin_unlock(&isotp_notifier_lock);
>  
> +	rtnl_lock();
>  	lock_sock(sk);
>  
>  	/* remove current filters & unregister */
> -	if (so->bound) {
[ ... ]
> +	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->dev = NULL;
> +		synchronize_rcu();

[Severity: High]
Does holding rtnl_lock() in isotp_release() (net/can/isotp.c) across
synchronize_rcu() cause severe latency spikes and potential denial of
service system-wide?

synchronize_rcu() blocks until all currently active RCU read-side critical
sections across the system finish, which stalls all other network
configuration operations while rtnl_lock() is held.

[Severity: Critical]
This is a pre-existing issue, but skipping synchronize_rcu() when the socket
is already unbound (e.g., by NETDEV_UNREGISTER where isotp_notify() sets
so->bound = 0) appears to allow a use-after-free.

If synchronize_rcu() is bypassed, isotp_release() proceeds to cancel the
socket's hrtimers and drop the user reference without waiting for concurrent
receive operations to complete.

Could a concurrent CAN receive operation like isotp_rcv_ff() or
isotp_rcv_echo() executing within an RCU read-side critical section
unconditionally re-arm the hrtimer after isotp_release() has cancelled it,
leading to a use-after-free when the timer later fires on the freed socket
memory?

>  	}
>  
>  	hrtimer_cancel(&so->txfrtimer);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616123915.674534-1-vasilisalmpanis@gmail.com?part=1

Re: [PATCH] can: isotp: hold a reference to the bound netdevice

[Vasileios Almpanis]

On 6/16/26 2:53 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Calling synchronize_rcu() while holding rtnl_lock() introduces severe latency spikes and potential denial of service system-wide.
>
> Pre-existing issues:
> - [Critical] Skipping synchronize_rcu() in isotp_release() when the socket is unbound leads to a use-after-free of the socket structure by re-armed hrtimers.
> --
>
> commit da93bcd8877d85ffe0b043e3045c3374b10e38e5
> Author: Vasileios Almpanis<vasilisalmpanis@gmail.com>
>
> can: isotp: hold a reference to the bound netdevice
>      
> isotp_release() looked up the bound netdevice with
> dev_get_by_index(so->ifindex) and only unregistered the socket's CAN
> receivers if that lookup succeeded. This is unreliable while the device
> is going away.
>
> Cc:stable@vger.kernel.org
> Closes:https://syzkaller.appspot.com/bug?id=58676a0f698531996a42612c552e894a55b9732b
>
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index c48b4a818297e..308d18040b6db 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -1219,27 +1222,22 @@ static int isotp_release(struct socket *sock)
>>   	list_del(&so->notifier);
>>   	spin_unlock(&isotp_notifier_lock);
>>   
>> +	rtnl_lock();
>>   	lock_sock(sk);
>>   
>>   	/* remove current filters & unregister */
>> -	if (so->bound) {
> [ ... ]
>> +	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->dev = NULL;
>> +		synchronize_rcu();
> [Severity: High]
> Does holding rtnl_lock() in isotp_release() (net/can/isotp.c) across
> synchronize_rcu() cause severe latency spikes and potential denial of
> service system-wide?
>
> synchronize_rcu() blocks until all currently active RCU read-side critical
> sections across the system finish, which stalls all other network
> configuration operations while rtnl_lock() is held.
Okay this is indeed true. rtnl_lock/unlock is not really necessary as 
now receivers will be tracked
through *dev_tracker*. I wanted to mimic *raw_release() *but maybe too 
faithfully. Just dropping rtnl_lock/unlock from
*isotp_release* and *isotp_bind* will be enough since they are 
serialized under *lock_sock.*
> *[*Severity: Critical]
> This is a pre-existing issue, but skipping synchronize_rcu() when the socket
> is already unbound (e.g., by NETDEV_UNREGISTER where isotp_notify() sets
> so->bound = 0) appears to allow a use-after-free.
>
> If synchronize_rcu() is bypassed, isotp_release() proceeds to cancel the
> socket's hrtimers and drop the user reference without waiting for concurrent
> receive operations to complete.
>
> Could a concurrent CAN receive operation like isotp_rcv_ff() or
> isotp_rcv_echo() executing within an RCU read-side critical section
> unconditionally re-arm the hrtimer after isotp_release() has cancelled it,
> leading to a use-after-free when the timer later fires on the freed socket
> memory?
This is a pre-existing issue that is not touched by my changes so it's 
out of scope of this patch. Its true that timers could potentially be 
rearmed after being canceled and *synchronize_rcu *should be called 
unconditionally but I would do that in a separate patch for it to be 
fully bisectable.
Looking forward to any comment reviewers might have before sending v2.
>>   	}
>>   
>>   	hrtimer_cancel(&so->txfrtimer);