Re: [PATCH AUTOSEL 6.18] tty: serial: 8250: protect against NULL uart->port.dev in register

[sashiko-bot@kernel.org]

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

New issues:
- [High] Skipping `mctrl_gpio_init()` and `uart_get_rs485_mode()` when `uart->port.dev` is NULL exposes a Use-After-Free vulnerability due to dangling pointers in the reused static port slot.

Pre-existing issues:
- [High] The patch adds a NULL check for `uart->port.dev` before `has_acpi_companion()`, but leaves an unprotected dereference in `pm_runtime_get_sync()` just a few lines down.
- [Medium] The runtime PM usage counter is leaked if `uart_add_one_port()` fails.
--

commit c7d8108ae7556bad9c5dd70639d77621e2028aef
Author: Stepan Ionichev <sozdayvek@gmail.com>

tty: serial: 8250: protect against NULL uart->port.dev in register

This commit adds a NULL check for uart->port.dev before calling
has_acpi_companion(). This prevents a kernel oops for legacy callers that
legitimately pass no parent device.

Link: https://patch.msgid.link/20260508181237.11146-1-sozdayvek@gmail.com

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index bfa421ab32536..8c169756710b5 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -760,7 +760,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  	 * Only call mctrl_gpio_init(), if the device has no ACPI
>  	 * companion device
>  	 */
> -	if (!has_acpi_companion(uart->port.dev)) {
> +	if (uart->port.dev && !has_acpi_companion(uart->port.dev)) {

[Severity: High]
Does skipping this initialization leave dangling pointers if the slot was
previously used?

uart->gpios and the RS485 GPIOs are devres-managed memory tied to the port's
device. If a port is unregistered, serial8250_unregister_port() sets
uart->port.dev to NULL and the devm memory is freed, but the global
serial8250_ports array retains the dangling pointers.

If the slot is subsequently reused by a legacy caller without a device, this
patched code skips mctrl_gpio_init() and uart_get_rs485_mode() without
clearing the old pointers. When uart_add_one_port() is called,
serial8250_set_mctrl() will dereference the freed gpios pointer.

Before this patch, this use-after-free was unreachable because the
unconditional has_acpi_companion() check would oops first. By bypassing the
oops, this patch makes the use-after-free reachable.

[Severity: High]
This isn't a bug introduced by this patch, but does this leave an unprotected
dereference just a few lines further down?

If a legacy caller registers a port over an active console without a parent
device, which this patch intends to support, we reach this code:

	if (uart_console_registered(&uart->port))
		pm_runtime_get_sync(uart->port.dev);

Since __pm_runtime_resume() unconditionally dereferences dev->power
internally, would this crash on the NULL pointer when pm_runtime_get_sync()
is called?

[Severity: Medium]
This is a pre-existing issue, but does this code leak the runtime PM usage
counter if uart_add_one_port() fails?

If the port is registered as a console, pm_runtime_get_sync() increments the
PM usage counter. If the subsequent uart_add_one_port() call fails, the
function jumps to the error handler:

	if (uart_console_registered(&uart->port))
		pm_runtime_get_sync(uart->port.dev);

	...
	ret = uart_add_one_port(&serial8250_reg, &uart->port);
	if (ret)
		goto err;
	...

err:
	uart->port.dev = NULL;
	return ret;

Since pm_runtime_put_sync() is never called on this error path, does this
prevent the device from ever suspending?

>  		struct mctrl_gpios *gpios = mctrl_gpio_init(&uart->port, 0);
>  		if (IS_ERR(gpios)) {
>  			ret = PTR_ERR(gpios);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-77-sashal@kernel.org?part=1