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

[Jiri Slaby <jirislaby@kernel.org>]

On 14. 10. 26, 8:49, sashiko-bot@kernel.org wrote:
> 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;

Am I hallucinating or you do? I have:
   return -ENOMEM;
on that line.

>>   	}
>>   
>>   	/* 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.
> 


-- 
js
suse labs