Re: [PATCH v2 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:
- [Critical] Forcibly resetting `so->tx.state = ISOTP_IDLE` in `isotp_notify()` breaks the lockless mutual exclusion mechanism used by `isotp_sendmsg()`, leading to concurrent execution, memory corruption, and an Out-of-Bounds (OOB) write.
--

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

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

This patch fixes a use-after-free race condition by holding a tracked
reference to the bound net_device in the socket from bind() onward. It
serializes bind()/release() with rtnl_lock() and resets the tx/rx state
machine and hrtimers in isotp_notify() during NETDEV_UNREGISTER.

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

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

[ ... ]

> @@ -1582,10 +1599,30 @@ 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;
> +
> +		/* wait for in-flight isotp_rcv()/isotp_rcv_echo() calls to
> +		 * finish so they can't leak stale tx data via a re-armed
> +		 * timer after a rebind.
> +		 * synchronize_net() is expedited here since rtnl_lock is held
> +		 */
> +		synchronize_net();
> +
> +		hrtimer_cancel(&so->txfrtimer);
> +		hrtimer_cancel(&so->txtimer);
> +		hrtimer_cancel(&so->rxtimer);
> +
> +		/* reset tx/rx state so a following bind() starts clean */
> +		so->tx.state = ISOTP_IDLE;
> +		so->rx.state = ISOTP_IDLE;

[Severity: Critical]
Does forcibly resetting so->tx.state to ISOTP_IDLE here break the lockless
mutual exclusion mechanism used by isotp_sendmsg()?

isotp_sendmsg() relies on cmpxchg() to protect the state machine without
holding lock_sock():

	while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {

If a thread is actively sleeping inside isotp_sendmsg() (for instance,
waiting for buffer space in sock_alloc_send_skb()), a concurrent
NETDEV_UNREGISTER will invoke isotp_notify() which clears the state here.

Because the state is externally reset, a subsequent bind() and sendmsg()
could successfully claim ISOTP_SENDING again, allowing two threads to execute
isotp_sendmsg() concurrently on the same socket.

Could this concurrent execution corrupt the shared so->tx state machine,
allowing so->tx.idx to exceed so->tx.len?

If so, later in isotp_send_cframe(), the space calculation would become
negative:

		space = so->tx.len - so->tx.idx;
		for (i = space; i < so->ll.tx_dl - SF_PCI_SZ4 - ae - off; i++)
			cf->data[SF_PCI_SZ4 + ae + off + i] = so->opt.txpad_content;

Can this cause a backward out-of-bounds write into the SKB headroom?

> +		so->cfecho = 0;
> +		wake_up_interruptible(&so->wait);
> +
>  		release_sock(sk);
>  
>  		sk->sk_err = ENODEV;

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