Re: [PATCH v5 2/4] 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] Missing rx.state validation in isotp_bind() allows cross-interface data corruption and spurious timeouts.
--

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

This patch fixes a use-after-free race by holding a tracked reference to
the bound net_device in the socket from bind() onward. It also serializes
bind() and release() with rtnl_lock() and adds a check to reject binding
if the transmission state is not idle.

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

> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1317,6 +1327,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> +	/* A transmission that outlived a previous binding (unbound by
> +	 * NETDEV_UNREGISTER) may still be draining; the FC/echo watchdog
> +	 * timers bound how long this takes. Checked together with
> +	 * so->bound in the same lock_sock() section above, so there is no
> +	 * window in which a concurrent isotp_notify() could be missed.
> +	 */
> +	if (so->tx.state != ISOTP_IDLE) {
> +		err = -EAGAIN;
> +		goto out;
> +	}
> +

[Severity: High]
This is a pre-existing issue, but should we also validate so->rx.state here?

If a multi-frame ISOTP message is being received (ISOTP_WAIT_DATA state
with rxtimer armed) during a concurrent NETDEV_UNREGISTER, the socket is
unbound but so->rx.state and rxtimer remain active.

If the user quickly re-binds to a new interface, would consecutive CAN frames
on the newly bound interface be erroneously appended to the old buffer from
the deleted interface?

Could this also cause the legacy rxtimer to spuriously set sk_err = ETIMEDOUT
on the new connection?

>  	/* ensure different CAN IDs when the rx_id is to be registered */
>  	if (isotp_register_rxid(so) && rx_id == tx_id) {
>  		err = -EADDRNOTAVAIL;

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