Re: [PATCH v3 2/2] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless check of `so->bound` in `isotp_bind()` creates a TOCTOU race that bypasses the newly added wait for `ISOTP_IDLE`, allowing cross-device packet injection.
--

--- Patch [2]: [PATCH v3 2/2] can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER ---
commit e4c5bbbd972ba76bfee06412e3abf69c164cec65
Author: Oliver Hartkopp <[email protected]>
can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER

This commit fixes a use-after-free race in isotp_release() and isotp_notify() 
by taking a tracked reference to the net_device and serializing bind/release 
with rtnl_lock(). It also adds a wait in isotp_bind() to ensure any active 
transmissions settle if the socket was unbound by a concurrent device 
unregistration.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index d30937345bcdc..d98863bad0d9f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1310,6 +1319,18 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  	if (!addr->can_ifindex)
>  		return -ENODEV;
>  
> +	if (!so->bound) {
> +		/* A transmission that outlived a previous binding (unbound
> +		 * by NETDEV_UNREGISTER) must settle before so->tx can be
> +		 * reused; the FC/echo watchdog timers bound this wait.
> +		 */
> +		err = wait_event_interruptible(so->wait,
> +					       so->tx.state == ISOTP_IDLE);
> +		if (err)
> +			return err;
> +	}
> +
> +	rtnl_lock();
>  	lock_sock(sk);
>  
>  	if (so->bound) {

[Severity: High]
Can this lockless check of so->bound bypass the wait if the socket is 
unbound just before acquiring the locks?

If a user initiates a bind to a new device while the socket is currently 
bound and transmitting, the initial if (!so->bound) check will be false, 
skipping the wait for ISOTP_IDLE. 

If a concurrent NETDEV_UNREGISTER runs and clears so->bound before this 
thread acquires rtnl_lock(), the inner if (so->bound) check will then 
evaluate to false. 

Does this allow the bind to succeed while the state machine is still 
transmitting, potentially injecting the remaining payload onto the newly 
bound CAN device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2