Re: [PATCH v6 2/5] 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:

Pre-existing issues:
- [High] The wait loop error path in `isotp_sendmsg()` resets the transmission state machine despite not owning it, which can silently corrupt another thread's active transmission and lead to an information leak.
--

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

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

This commit fixes a race condition during device unregistration by holding a
tracked reference to the bound net_device and serializing bind and release
with rtnl_lock().

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> --- 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;

[Severity: High]
This is a pre-existing issue, but does this error path corrupt the state
machine for another thread?

If Thread A acquires the state (so->tx.state = ISOTP_SENDING) and starts
transmission, and Thread B calls isotp_sendmsg(), Thread B will fail the
cmpxchg() and block in wait_event_interruptible().

If Thread B receives a signal, it jumps to err_event_drop and forces 
so->tx.state = ISOTP_IDLE, even though Thread A still owns the active 
transmission.

Could Thread C then call isotp_sendmsg(), acquire the now-IDLE state
machine, and begin setting up a new transmission? If a delayed flow
control frame for Thread A's transmission arrives, it seems the state
machine could mistakenly transmit consecutive frames containing the newly
allocated, uninitialized memory from Thread C onto the CAN bus.

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

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