Re: [PATCH 1/3] serial: 8250_lpss: do not touch uninitialized line in ehl_serial_exit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serial: 8250_lpss: race in remove path corrupts reallocated port DMA
- [High] tty: serial8250: UAF due to dangling dma pointer on register failure or unregister
--

commit 1d475b7f1e61baf36ab1328820f63e24baee9a1a
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

serial: 8250_lpss: do not touch uninitialized line in ehl_serial_exit()

This commit fixes an issue in lpss8250_probe() where lpss->board->exit()
could clear the dma pointer of an unrelated port on the error path. It
initializes lpss->data.line to -1 and skips clearing the DMA pointer in
ehl_serial_exit() if no port was registered.

> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 776ec1ef29d62..c5257847a94c8 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -183,6 +183,9 @@ static int ehl_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>  
>  static void ehl_serial_exit(struct lpss8250 *lpss)
>  {
> +	if (lpss->data.line < 0)
> +		return;
> +
>  	struct uart_8250_port *up = serial8250_get_port(lpss->data.line);
>  
>  	up->dma = NULL;

[Severity: High]
Does this locklessly clear the DMA pointer on a potentially reallocated port
slot?

In lpss8250_remove(), the port is unregistered before the board exit callback
cleans up the DMA pointer:

drivers/tty/serial/8250/8250_lpss.c:lpss8250_remove()
    serial8250_unregister_port(lpss->data.line);
    lpss->board->exit(lpss);

Because serial8250_unregister_port() clears the port's registration (setting
uart->port.dev = NULL) and releases serial_mutex, the global
serial8250_ports[] array slot becomes available.

If a concurrent driver probes a different 8250 serial device, claims the
same slot via serial8250_register_8250_port(), and assigns its own uport->dma
pointer, won't up->dma = NULL in ehl_serial_exit() locklessly zero out the
newly assigned DMA pointer, leading to a NULL pointer dereference or loss of
DMA functionality for the new device?

> [ ... ]
> @@ -329,6 +332,8 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return ret;
>  
>  	lpss->board = (struct lpss8250_board *)id->driver_data;
> +	/* for tests in lpss->board->exit() */
> +	lpss->data.line = -1;
>  
>  	memset(&uart, 0, sizeof(struct uart_8250_port));
>  

[Severity: High]
Does the failure path of serial8250_register_8250_port() leave a dangling
DMA pointer?

If serial8250_register_8250_port() fails (e.g. uart_add_one_port fails)
when called from probe, it jumps to its error path:

drivers/tty/serial/8250/8250_core.c:serial8250_register_8250_port()
err:
    uart->port.dev = NULL;
    return ret;

This nullifies the device pointer but leaves uart->dma pointing to memory
allocated during the caller's probe (e.g. devm_kzalloc). When the probe
fails and devm frees this memory, doesn't it leave a dangling pointer in
the global serial8250_ports array?

Similarly, if a module successfully registers a port but is unloaded before
the port is ever opened, serial8250_unregister_port() will not clear
uart->dma (as serial8250_do_shutdown is skipped for unopened ports) and the
module's memory is freed.

If an administrator unbinds and rebinds the serial8250 platform driver, or
the core automatically re-registers the port under serial8250_isa_devs,
couldn't an unprivileged user trigger serial8250_startup() ->
serial8250_request_dma() and dereference the freed pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ef47d676a44346e058f29a00c8701484db722831.1791524724.git.jirislaby@kernel.org?part=1