[PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[Oliver Hartkopp]

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] 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] Skipped CAN filter unregistration during concurrent device removal leads to Use-After-Free and memory leak.
--

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

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

This commit addresses a race condition in the timer callback of isotp
sockets. It enforces an unconditional synchronize_rcu() in isotp_release()
before cancelling timers to avoid issues during concurrent
NETDEV_UNREGISTER notifications.

Closes: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@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

[ ... ]

> @@ -1227,14 +1227,20 @@ static int isotp_release(struct socket *sock)
>  	/* 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);

[Severity: Critical]
This is a pre-existing issue, but could this dev_get_by_index() call return
NULL if a device unregistration is running concurrently?

If unregister_netdevice_many() unlists the device before the
NETDEV_UNREGISTER notifier executes, and isotp_release() is called at the
same time, it removes the socket from the isotp notifier list.

When it calls dev_get_by_index() here, it will return NULL because the
device is unlisted. The if (dev) condition fails and we skip calling
can_rx_unregister().

The NETDEV_UNREGISTER notifier will miss this socket because it was already
removed from the notifier list.

Since the socket is freed at the end of this function, does this leave an
active CAN filter pointing to a freed socket? Any delayed or looped-back
CAN frames processed during device teardown could invoke isotp_rcv() on
this freed socket, causing a use-after-free and a memory leak.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707094716.63578-1-socketcan@hartkopp.net?part=1

Re: [PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[Oliver Hartkopp]

Hi Marc,

do you consider to upstream this patch together with the BCM patch set?

I know sashiko-bot had again some "Pre-existing issue" remarked on this 
patch. But this simple fix should probably go into upstream, before the 
next "AI Pre-existing issue"-party starts with isotp.c and sashiko-bot ;-)

Best regards,
Oliver

On 07.07.26 11:47, Oliver Hartkopp wrote:
> 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;