Re: [PATCH] usb: sl811-hcd: delete the driver timer on stop to avoid use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] usb: sl811-hcd: unlocked read of sl811->ctrl1 in
  sl811h_timer() [1]
--

commit 2d5a12c739030030cdd0a7f7bbfa2df343bb1b51
Author: Hyunjung Ko <hj351016@gmail.com>

usb: sl811-hcd: delete the driver timer on stop to avoid use-after-free

This commit calls timer_delete_sync(&sl811->timer) in sl811h_stop() before
halting the controller to prevent sl811h_timer() from running after the
HCD private data is freed.

> diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
> index b044977f6f567..b667732f67de6 100644
> --- a/drivers/usb/host/sl811-hcd.c
> +++ b/drivers/usb/host/sl811-hcd.c
> @@ -1516,6 +1516,7 @@ sl811h_stop(struct usb_hcd *hcd)
>  	unsigned long	flags;
>  
>  	timer_delete_sync(&hcd->rh_timer);
> +	timer_delete_sync(&sl811->timer);
>  
>  	spin_lock_irqsave(&sl811->lock, flags);
>  	port_power(sl811, 0);

[Severity: Medium]
This problem wasn't introduced by this patch, but can sl811h_timer() race
with port_power() because it reads sl811->ctrl1 before taking sl811->lock
[1]?

drivers/usb/host/sl811-hcd.c:sl811h_timer() {
	...
	u8		signaling = sl811->ctrl1 & SL11H_CTL1MASK_FORCE;
	const u32	mask = USB_PORT_STAT_CONNECTION
				| USB_PORT_STAT_ENABLE
				| USB_PORT_STAT_LOW_SPEED;

	spin_lock_irqsave(&sl811->lock, flags);
	...
}

If sl811h_hub_control() powers off the port while the timer fires,
sl811h_timer() can observe a stale SL11H_CTL1MASK_SE0 value and re-enable
USB_PORT_STAT_POWER in sl811->port1:

CPU0                            CPU1
sl811h_timer()
  signaling = sl811->ctrl1 & ...
                                sl811h_hub_control()
                                  spin_lock_irqsave(&sl811->lock, ...)
                                  port_power(sl811, 0)
                                    sl811->port1 = 0;
                                    sl811->ctrl1 = 0;
                                  spin_unlock_irqrestore(&sl811->lock, ...)
  spin_lock_irqsave(&sl811->lock, ...)
  switch (signaling)
  case SL11H_CTL1MASK_SE0:
    sl811->port1 = ...;         <-- acts on stale signaling state

[1] https://sashiko.dev/#/bug/linux-8a163ef9-3fa5-4e03-b20e-6da57ee81b06

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107151847.3754846-1-hj351016@gmail.com?part=1