Re: [PATCH 3/3] serial: 8250: make port operations work on struct uart_8250_port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: NULL/ERR_PTR dereference in pci_xr17v35x_exit() on probe failure
- [High] tty: NULL pointer dereference in omap8250_irq() during device removal
- [Medium] tty: cleanup mixing goto with guard(mutex) in serial8250_register_8250_port
--

commit 945c4e51972de691504f65b6a2de3089490c457d
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

serial: 8250: make port operations work on struct uart_8250_port

serial8250_register_8250_port() returns a line number and the other
port operations (serial8250_unregister_port(), serial8250_suspend_port(),
serial8250_resume_port()) accept it. Many drivers then convert the line
back to the port using serial8250_get_port() anyway.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 11424fef7c08b..6ca6b3bed80f5 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -754,16 +751,16 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
>   *	The port is then probed and if necessary the IRQ is autodetected
>   *	If this fails an error is returned.
>   *
> - *	On success the port is ready to use and the line number is returned.
> + *	On success, the port is returned as ready to use.
>   */
> -int serial8250_register_8250_port(const struct uart_8250_port *up)
> +struct uart_8250_port *serial8250_register_8250_port(const struct uart_8250_port *up)
>  {
>  	struct uart_8250_port *uart;
>  	bool cons_flow;
>  	int ret;
>  
>  	if (up->port.uartclk == 0)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  
>  	guard(mutex)(&serial_mutex);
>  
[ ... ]
> @@ -926,25 +919,23 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  		uart->overrun_backoff_time_ms = 0;
>  	}
>  
> -	return ret;
> +	return uart;
>  
>  err:
>  	uart->port.dev = NULL;
> -	return ret;
> +	return ERR_PTR(ret);
>  }

[Severity: Medium]
Does this code mix goto-based error handling with scope-based lock cleanup?

According to the cleanup guidelines, mixing goto-based error handling and
guard(mutex) within the same function creates confusing ownership semantics.
Since this function uses guard(mutex)(&serial_mutex), it might be better to
convert the entire function to use scope-based cleanup instead of retaining
the err label.

> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 836792e861bfc..453e843b78ca1 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -1377,9 +1377,8 @@ static void pci_xr17v35x_exit(struct pci_dev *pcidev)
>  {
>  	const struct exar8250_platform *platform = exar_get_platform();
>  	struct exar8250 *priv = pci_get_drvdata(pcidev);
> -	struct uart_8250_port *port = serial8250_get_port(priv->line[0]);
>  
> -	platform->unregister_gpio(port);
> +	platform->unregister_gpio(priv->uport[0]);
>  }

[Severity: High]
Could this result in a NULL or ERR_PTR dereference during unbind if the device
probe failed for the first port?

If board->setup() fails for the first port (i=0) in exar_pci_probe(),
priv->uport[0] remains NULL. If serial8250_register_8250_port() fails, it
returns an ERR_PTR. In either case, the loop breaks and probe returns
success. When the device is later unbound, pci_xr17v35x_exit()
unconditionally passes priv->uport[0] to platform->unregister_gpio(),
which will dereference it without checking for validity.

> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 137175aa4183a..3eb69a2f26e1f 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -628,7 +628,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port);
>  static irqreturn_t omap8250_irq(int irq, void *dev_id)
>  {
>  	struct omap8250_priv *priv = dev_id;
> -	struct uart_8250_port *up = serial8250_get_port(priv->line);
> +	struct uart_8250_port *up = priv->uport;
>  	struct uart_port *port = &up->port;
>  	unsigned int iir, lsr;
>  	int ret;

[ ... ]

> @@ -1624,10 +1625,10 @@ static void omap8250_remove(struct platform_device *pdev)
>  	if (err)
>  		dev_err(&pdev->dev, "Failed to resume hardware\n");
>  
> -	up = serial8250_get_port(priv->line);
> +	up = priv->uport;
>  	omap_8250_shutdown(&up->port);
> -	serial8250_unregister_port(priv->line);
> -	priv->line = -ENODEV;
> +	serial8250_unregister_port(up);
> +	priv->uport = NULL;
>  	dev_pm_clear_wake_irq(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	pm_runtime_put_sync(&pdev->dev);

[Severity: High]
Does this code introduce a NULL pointer dereference if an interrupt fires
during device removal?

In omap8250_remove(), priv->uport is set to NULL after the port is
unregistered. Because the IRQ handler is managed by devm_request_irq(), it
remains active until devres cleanup runs after the remove function completes.
If an interrupt fires in this window, omap8250_irq() will read priv->uport
(which is now NULL) and dereference it when accessing up->port.

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