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 1 potential
issue to consider:
- [Medium] usb: sl811-hcd: sl811h_stop() deletes timer after
  port_power() halt
--

commit 2938f64d61ba1aa6f4f9800e4e15b39f1ccc6b42
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() so that a
pending port timer does not fire after the HCD and struct sl811 are freed.

> diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
> index b044977f6f567..53a0bc81916ad 100644
> --- a/drivers/usb/host/sl811-hcd.c
> +++ b/drivers/usb/host/sl811-hcd.c
> @@ -1520,6 +1520,8 @@ sl811h_stop(struct usb_hcd *hcd)
>  	spin_lock_irqsave(&sl811->lock, flags);
>  	port_power(sl811, 0);
>  	spin_unlock_irqrestore(&sl811->lock, flags);
> +
> +	timer_delete_sync(&sl811->timer);
>  }
>  
>  static int

[Severity: Medium]
Can sl811h_timer() still run in the window between spin_unlock_irqrestore()
and timer_delete_sync(&sl811->timer), re-enabling the controller after
port_power(sl811, 0) has halted it?

CPU0                                    CPU1 (<softirq>)
sl811h_stop()
  spin_lock_irqsave(&sl811->lock, ...)
  port_power(sl811, 0)
    hcd->state = HC_STATE_HALT;
    sl811_write(..., SL11H_IRQ_ENABLE, 0);
                                        sl811h_timer()
                                          spin_lock_irqsave(&sl811->lock, ...)
  spin_unlock_irqrestore(&sl811->lock, ...)
                                          sl811->ctrl1 |= SL11H_CTL1MASK_SOF_ENA;
                                          sl811_write(..., SL11H_HOSTCTLREG,
                                                      SL11H_HCTLMASK_ARM);
                                          sl811_write(..., SL11H_IRQ_ENABLE,
                                                      sl811->irq_enable);
                                            <-- re-arms halted controller
  timer_delete_sync(&sl811->timer)

sl811h_timer() does not check whether the port is powered or whether
hcd->state is HC_STATE_HALT.  If a port reset or resume timer expires while
port_power(sl811, 0) is running (which holds sl811->lock across a 20 ms
mdelay() when board->reset is NULL), sl811h_timer() runs as soon as
sl811->lock is released and re-enables SOF generation, Endpoint A, and
hardware interrupts in SL11H_IRQ_ENABLE on the halted controller.

Only sl811h_hub_control() arms sl811->timer (the transfer scheduler never
touches it), and usb_remove_hcd() has already stopped root-hub activity before
calling sl811h_stop().  Deleting sl811->timer alongside hcd->rh_timer before
port_power(sl811, 0) would drain any pending callback before halting the
hardware; the commit message should also be updated not to mention the
transfer scheduler.

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