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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

sl811h_hub_control() and the transfer scheduler arm sl811->timer
(sl811h_timer) with mod_timer(); the timer callback dereferences the
hcd-private struct sl811 (sl811->ctrl1, sl811->lock, sl811->port1, and
register I/O via sl811).

sl811h_stop() deletes only the USB core's root-hub timer
(hcd->rh_timer); it never deletes sl811->timer, and nothing else does
either. struct sl811 lives in the HCD private area and is freed by
usb_put_hcd() after sl811h_remove() calls usb_remove_hcd(). If
sl811->timer is still pending when the controller is stopped and the
HCD is freed, sl811h_timer() runs against the freed struct sl811
(use-after-free).

Delete sl811->timer in sl811h_stop(), after the port is powered down so
it cannot be re-armed, mirroring the existing hcd->rh_timer teardown.
sl811h_timer() does not re-arm itself, so a single timer_delete_sync()
drains it.

Found by static analysis and code review; not tested on real hardware
(needs an SL811HS USB host controller).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 drivers/usb/host/sl811-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index b044977..53a0bc8 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
-- 
2.43.0

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

[sashiko-bot]

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

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

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

sl811h_hub_control() arms sl811->timer (sl811h_timer) with mod_timer()
for port reset/resume timing; the timer callback dereferences the
hcd-private struct sl811 (sl811->ctrl1, sl811->lock, sl811->port1, and
register I/O via sl811).

sl811h_stop() deletes only the USB core's root-hub timer
(hcd->rh_timer); it never deletes sl811->timer, and nothing else does
either. struct sl811 lives in the HCD private area and is freed by
usb_put_hcd() after sl811h_remove() calls usb_remove_hcd(). If
sl811->timer is still pending when the controller is stopped and the HCD
is freed, sl811h_timer() runs against the freed struct sl811
(use-after-free).

Delete sl811->timer at the start of sl811h_stop(), next to the existing
hcd->rh_timer teardown and before port_power(sl811, 0) halts the
controller, so any pending sl811h_timer() is drained before the hardware
is halted and cannot re-enable it afterwards. usb_remove_hcd() has
already stopped root-hub activity before sl811h_stop() runs, so the timer
cannot be re-armed, and sl811h_timer() does not re-arm itself, so a
single timer_delete_sync() drains it.

Found by static analysis and code review; not tested on real hardware
(needs an SL811HS USB host controller).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
v2: delete sl811->timer BEFORE port_power(sl811, 0) (top of sl811h_stop(),
    next to hcd->rh_timer) instead of after it, so a pending sl811h_timer()
    is drained before the controller is halted and cannot re-enable it in
    the window after the lock is dropped. Corrected the log: only
    sl811h_hub_control() arms sl811->timer (not the transfer scheduler).
    Both per Sashiko AI review.

 drivers/usb/host/sl811-hcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index b044977..b667732 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);
-- 
2.43.0

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

[sashiko-bot]

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