Re: [PATCH v4 3/7] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Error paths in `aspeed_vuart_probe()` after serial port registration fail to unregister the port, leading to a Use-After-Free vulnerability.
- [High] Incorrect teardown ordering in `aspeed_vuart_remove()` allows the unthrottle timer to be re-armed after it is deleted, leading to a Use-After-Free vulnerability.
--

--- Patch [3]: [PATCH v4 3/7] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string ---
commit be545f82b6eac2794c78d897d8e0ac9945c563d1
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string
    
Make the driver compatible with the ast2600-vuart.
Enable specific configuration for the AST2600.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 26fc0464f1cc2..6afa2f4057e1b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
>  static const struct of_device_id aspeed_vuart_table[] = {
>  	{ .compatible = "aspeed,ast2400-vuart" },
>  	{ .compatible = "aspeed,ast2500-vuart" },
> +	{ .compatible = "aspeed,ast2600-vuart" },
>  	{ },
>  };

[Severity: High]
This isn't a bug introduced by this patch, but the error paths in
aspeed_vuart_probe() after serial port registration fail to unregister
the port.

If an initialization step fails after serial8250_register_8250_port() (e.g.
if aspeed_vuart_set_lpc_address() returns -EINVAL), the code jumps here:

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
err_sysfs_remove:
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
	return rc;
}

This error path omits serial8250_unregister_port(vuart->line) and
timer_delete_sync(&vuart->unthrottle_timer), leaking the registered port.

Since the vuart struct is then freed by devres, if a user opens the leaked
serial port, aspeed_vuart_startup() will dereference the freed vuart struct.

Should the error path unregister the port and delete the timer?


[Severity: High]
This isn't a bug introduced by this patch, but there is an incorrect teardown
ordering in aspeed_vuart_remove() that allows the unthrottle timer to be
re-armed after it is deleted.

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_remove() {
	struct aspeed_vuart *vuart = platform_get_drvdata(pdev);

	timer_delete_sync(&vuart->unthrottle_timer);
	aspeed_vuart_set_enabled(vuart, false);
	serial8250_unregister_port(vuart->line);
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
}

Because timer_delete_sync() is invoked before serial8250_unregister_port(),
the UART and its IRQ are still fully active. An incoming interrupt can trigger
aspeed_vuart_handle_irq(), and if the tty buffer is full, re-arm the timer
via mod_timer(). 

When the driver removal completes, the vuart struct is freed by devres. When
the re-armed timer fires, aspeed_vuart_unthrottle_exp() will dereference the
freed vuart struct.

Could the timer be deleted after unregistering the serial port to prevent
it from being re-armed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783524645.git.gregoire.layet@9elements.com?part=3