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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: serial: 8250_hp300: NULL pointer dereference on hpdca device removal
--

commit f913317bfee3afed5e144530ca60f25858464754
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Date:   Tue Oct 13 06:48:51 2026 +0000

    serial: 8250: make port operations work on struct uart_8250_port
    
    This commit switches the interface of port operations to use
    struct uart_8250_port instead of the line number, returning the registered
    port or an ERR_PTR() on failure. It updates all drivers to store the port
    pointer instead of the line number.

> diff --git a/drivers/tty/serial/8250/8250_hp300.c b/drivers/tty/serial/8250/8250_hp300.c
> index 59ec0c7377a1c..e9599c5c2fead 100644
> --- a/drivers/tty/serial/8250/8250_hp300.c
> +++ b/drivers/tty/serial/8250/8250_hp300.c
[ ... ]
> @@ -177,9 +177,8 @@ static int hpdca_init_one(struct dio_dev *d,
>  	uart.port.membase = (char *)(uart.port.mapbase + DIO_VIRADDRBASE);
>  	uart.port.regshift = 1;
>  	uart.port.dev = &d->dev;
> -	line = serial8250_register_8250_port(&uart);
> -
> -	if (line < 0) {
> +	uport = serial8250_register_8250_port(&uart);
> +	if (IS_ERR(uport)) {
>  		dev_notice(&d->dev,
>  			  "8250_hp300: register_serial() DCA scode %d irq %d failed\n",
>  			  d->scode, uart.port.irq);
>  		return 0;
>  	}
>  
>  	/* Enable board-interrupts */
>  	out_8(d->resource.start + DIO_VIRADDRBASE + DCA_IC, DCA_IC_IE);
> -	dio_set_drvdata(d, (void *)line);
> +	dio_set_drvdata(d, uport);
>  
>  	/* Reset the DCA */
>  	out_8(d->resource.start + DIO_VIRADDRBASE + DCA_ID, 0xff);

[Severity: High]
If serial8250_register_8250_port() fails here and returns an ERR_PTR, this
function logs a warning and returns 0 (success). In this failure path,
dio_set_drvdata() is skipped, leaving the device's driver data as NULL.

Because the driver returns 0, the device is considered successfully bound.
When the device is later removed, hpdca_remove_one() is called:

> @@ -287,14 +283,13 @@ static int __init hp300_8250_init(void)
>  #ifdef CONFIG_HPDCA
>  static void hpdca_remove_one(struct dio_dev *d)
>  {
> -	int line;
> +	struct uart_8250_port *uport = dio_get_drvdata(d);
>  
> -	line = (int) dio_get_drvdata(d);
>  	if (d->resource.start) {
>  		/* Disable board-interrupts */
>  		out_8(d->resource.start + DIO_VIRADDRBASE + DCA_IC, 0);
>  	}
> -	serial8250_unregister_port(line);
> +	serial8250_unregister_port(uport);
>  }
>  #endif

Will this lead to a NULL pointer dereference?

If the registration failed during probe, uport will be NULL here. Since
serial8250_unregister_port() unconditionally dereferences the pointer (e.g.,
checking uart->em485), passing NULL will cause a kernel panic. 

Should the failure path in hpdca_init_one() return an error code instead of
0, or should hpdca_remove_one() check for a valid uport before
unregistering?

Note: This replaces a prior bug where passing an uninitialized 0 line number
caused the driver to erroneously unregister ttyS0 instead of crashing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791873876.git.jirislaby@kernel.org?part=6