Re: [PATCH 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:

Pre-existing issues:
- [High] Missing state cleanup on NETDEV_UNREGISTER allows cross-interface data injection during socket rebind.
--

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

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

This commit fixes a use-after-free in isotp_release() 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() to ensure the device
pointer remains consistent with NETDEV_UNREGISTER events.

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

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

[ ... ]

> @@ -1354,16 +1354,25 @@ 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;
>  
> +	/* bind() ok -> hold a reference for so->dev so that isotp_release()
> +	 * can safely reach the device later, even if a concurrent
> +	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
> +	 */
> +	so->dev = dev;
> +	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does rebinding the socket here safely
handle old transmission state?

If a socket is unbound via a concurrent NETDEV_UNREGISTER, the state
machine (so->tx.state) isn't cleared. Could a subsequent bind() to a new
interface cause still-active hrtimers to transmit the remaining frames of
an old packet onto this newly bound CAN bus?

[ ... ]

> @@ -1582,10 +1591,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(so->dev, &so->dev_tracker);
>  		}
>  
>  		so->ifindex = 0;
>  		so->bound  = 0;
> +		so->dev = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does NETDEV_UNREGISTER need
to reset the transmission state machine or cancel active hrtimers here?

When isotp_notify() handles NETDEV_UNREGISTER, it clears so->bound and
so->dev, but leaves high-resolution timers (like isotp_txfr_timer_handler)
running. Is it possible for these active timers to inject cross-interface
data if the user immediately rebinds the socket?

>  		release_sock(sk);
>  
>  		sk->sk_err = ENODEV;

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