[PATCH] serial: 8250_aspeed_vuart: shut down the unthrottle timer on removal

[PATCH] serial: 8250_aspeed_vuart: shut down the unthrottle timer on removal

[Myeonghun Pak]

aspeed_vuart_remove() deletes unthrottle_timer while the port IRQ is
still active. An interrupt can re-arm the timer before port shutdown,
allowing its callback to access freed driver data.

Use timer_shutdown_sync() to prevent rearming. Keep it before
serial8250_unregister_port(), which releases the register mapping
used by the callback.

Found by a static analysis tool under development.

Compile-tested with W=1 for arm64 and x86_64.
Not tested on hardware.

Fixes: 5909c0bf9c7a ("serial/aspeed-vuart: Implement quick throttle mechanism")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---

 drivers/tty/serial/8250/8250_aspeed_vuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 452bef82bec2..0776675b8382 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -552,7 +552,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
 {
 	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
 
-	timer_delete_sync(&vuart->unthrottle_timer);
+	timer_shutdown_sync(&vuart->unthrottle_timer);
 	aspeed_vuart_set_enabled(vuart, false);
 	serial8250_unregister_port(vuart->port);
 	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);

base-commit: 630059a5a632377b9b7b487baccde92b7ae1dd47
-- 
2.43.0

Re: [PATCH] serial: 8250_aspeed_vuart: shut down the unthrottle timer on removal

[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:
- [Critical] serial: 8250: use-after-free and port leaks on probe error
  paths [1]
--

commit afd3348a7cc94ca4f33ad325f21afb53b427217e
Author: Myeonghun Pak <mhun512@gmail.com>

serial: 8250_aspeed_vuart: shut down the unthrottle timer on removal

This commit replaces timer_delete_sync() with timer_shutdown_sync() in
aspeed_vuart_remove() to prevent unthrottle_timer from being re-armed by
an interrupt before the port is unregistered.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 452bef82bec2a..0776675b83823 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -552,7 +552,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
>  {
>  	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);
>  
> -	timer_delete_sync(&vuart->unthrottle_timer);
> +	timer_shutdown_sync(&vuart->unthrottle_timer);
>  	aspeed_vuart_set_enabled(vuart, false);
>  	serial8250_unregister_port(vuart->port);
>  	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);

[Severity: Critical]
This problem wasn't introduced by this patch, but does the error path in
aspeed_vuart_probe() leave the port registered [1] and cause a
use-after-free of vuart?

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
	...
	struct uart_8250_port *uport = serial8250_register_8250_port(&port);
	if (IS_ERR(uport)) {
		rc = PTR_ERR(uport);
		goto err_sysfs_remove;
	}
	...
	rc = aspeed_vuart_set_lpc_address(vuart, prop);
	if (rc < 0) {
		dev_err_probe(dev, rc, "invalid value in aspeed,lpc-io-reg property\n");
		goto err_sysfs_remove;
	}
	...
err_sysfs_remove:
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
	return rc;
}

When aspeed_vuart_set_lpc_address() or a later step fails after
serial8250_register_8250_port(), err_sysfs_remove returns without calling
serial8250_unregister_port().  The driver core then frees the
devm-allocated vuart while the 8250 core still holds port.private_data
pointing to it, so a later interrupt or TTY access dereferences freed
memory.

[1] https://sashiko.dev/#/bug/linux-99bf5468-4b0d-497a-8419-674f926b3cdb

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