Re: [PATCH v2 1/3] serial: 8250_mxpcie: set the driver data before registering ports

[Jiri Slaby <jirislaby@kernel.org>]

Cc Crescent

Sashiko appears to be right.

On 01. 09. 26, 6:32, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing initialization of `up.port.port_id` causes all registered serial ports on a multi-port device to share ID 0 and corrupt each other's hardware state.
> - [High] Unprotected read-modify-write on shared hardware register in `mxpcie8250_set_interface` causes a race condition between concurrent operations on adjacent ports.
> --
> 
> commit 0dfacc505133057b054c4b8c8d42d557343c298b
> Author: Linmao Li <lilinmao@kylinos.cn>
> 
> serial: 8250_mxpcie: set the driver data before registering ports
> 
> This commit fixes a NULL pointer dereference in the mxpcie8250_rs485_config
> callback. It moves pci_set_drvdata() before port registration so the board
> data is available when the callback is invoked during initialization.
> 
>> diff --git a/drivers/tty/serial/8250/8250_mxpcie.c b/drivers/tty/serial/8250/8250_mxpcie.c
>> index ce873fbd62e93..c0e3517d4e4ec 100644
>> --- a/drivers/tty/serial/8250/8250_mxpcie.c
>> +++ b/drivers/tty/serial/8250/8250_mxpcie.c
>> @@ -542,6 +542,8 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>   
>>   	mxpcie8250_init_board(pdev, priv);
>>   
>> +	pci_set_drvdata(pdev, priv);
>> +
>>   	up.port.dev = dev;
>>   	up.port.irq = pdev->irq;
>>   	up.port.uartclk = MOXA_PUART_BASE_BAUD * 16;
>> @@ -578,7 +580,6 @@ static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id
>>   		}
>>   		priv->port[i].rx_trig_level = MOXA_PUART_RX_TRIG_DEFAULT;
>>   	}
>> -	pci_set_drvdata(pdev, priv);
>>   
>>   	return 0;
>>   }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the port
> registration loop, I noticed that mxpcie8250_setup_port() seems to leave
> up.port.port_id initialized to 0 for all ports. Because the up structure
> is zero-initialized, and up.port.port_id is never assigned the idx value,
> it remains 0.
> 
> When this zeroed structure is passed into serial8250_register_8250_port(),
> does this cause all registered ports on a multi-port device to share ID 0?
> If so, hardware callbacks like mxpcie8250_rs485_config() and
> mxpcie8250_startup() that use port->port_id as an index might inadvertently
> apply configuration changes to port 0 instead of the intended port.
> 
> [Severity: High]
> This is also a pre-existing issue, but looking at mxpcie8250_set_interface(),
> the shared hardware register controlling adjacent ports is updated using an
> unprotected read-modify-write:
> 
> mxpcie8250_set_interface()
>      ...
>      cval = ioread8(uir_addr);
> 
>      if (port_idx % 2)
>          FIELD_MODIFY(MOXA_ODD_RS_MASK, &cval, mode);
>      else
>          FIELD_MODIFY(MOXA_EVEN_RS_MASK, &cval, mode);
> 
>      iowrite8(cval, uir_addr);
> 
> Since this is called by mxpcie8250_rs485_config(), which is only protected
> by the per-port uart_port_lock, could concurrent ioctls on adjacent ports
> (like port 0 and port 1) race and corrupt this shared register byte?
> 


-- 
js
suse labs