Re: [PATCH v2 3/6] serial: 8250_exar: store the GPIO platform device in struct exar8250

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: 8250_exar: missing IS_ERR check causes invalid pointer deref
--

commit 6cdc2bc96a13008a8cfba8cca844ee2bb25786e6
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

serial: 8250_exar: store the GPIO platform device in struct exar8250

The GPIO platform device is now stored in struct exar8250::gpio instead of
port->port.private_data to fix an issue where port 0 might not exist on
unbind. The platform->register_gpio() and platform->unregister_gpio()
callbacks are updated to take priv as an argument.

Link: https://lore.kernel.org/all/sashiko-outbox-166269@kernel.org/

> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index ad2e5841fbf69..29b2a29ab6f37 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
[ ... ]
> @@ -1167,22 +1169,21 @@ static const struct software_node exar_gpio_node = {
>  	.properties = exar_gpio_properties,
>  };
>  
> -static int xr17v35x_register_gpio(struct pci_dev *pcidev, struct uart_8250_port *port)
> +static int xr17v35x_register_gpio(struct exar8250 *priv, struct pci_dev *pcidev,
> +				  struct uart_8250_port *port)
>  {
>  	if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
> -		port->port.private_data =
> -			__xr17v35x_register_gpio(pcidev, &exar_gpio_node);
> +		priv->gpio = __xr17v35x_register_gpio(pcidev, &exar_gpio_node);

[Severity: High]
Does __xr17v35x_register_gpio() return an ERR_PTR if
platform_device_register_full() fails?

If it does, we might be storing an error pointer directly into priv->gpio in
xr17v35x_register_gpio() without validating it with IS_ERR(). Because the
error is ignored, the device probe successfully continues.

>  
>  	return 0;
>  }
>  
> -static void xr17v35x_unregister_gpio(struct uart_8250_port *port)
> +static void xr17v35x_unregister_gpio(struct exar8250 *priv)
>  {
> -	if (!port->port.private_data)
> -		return;
> -
> -	__xr17v35x_unregister_gpio(port->port.private_data);
> -	port->port.private_data = NULL;
> +	if (priv->gpio) {

[Severity: High]
Later, during module unload or device unbind, we evaluate if (priv->gpio)
here in xr17v35x_unregister_gpio().

Since an ERR_PTR is not NULL, the condition passes and the error pointer is
passed to __xr17v35x_unregister_gpio().

Inside, device_remove_software_node(&pdev->dev) attempts to dereference the
invalid pointer, resulting in a crash. Could we add an IS_ERR check when
assigning priv->gpio, or check for it here?

> +		__xr17v35x_unregister_gpio(priv->gpio);
> +		priv->gpio = NULL;
> +	}
>  }
[ ... ]
> @@ -1289,8 +1291,7 @@ static int iot2040_register_gpio(struct pci_dev *pcidev,
>  	writeb(IOT2040_UARTS_ENABLE, p + UART_EXAR_MPIOLVL_15_8);
>  	writeb(IOT2040_UARTS_GPIO_HI_MODE, p + UART_EXAR_MPIOSEL_15_8);
>  
> -	port->port.private_data =
> -		__xr17v35x_register_gpio(pcidev, &iot2040_gpio_node);
> +	priv->gpio = __xr17v35x_register_gpio(pcidev, &iot2040_gpio_node);

[Severity: High]
This looks like the same concern as above in xr17v35x_register_gpio(). If
__xr17v35x_register_gpio() fails and returns an error pointer, we'll store
it here in iot2040_register_gpio() unchecked.

>  
>  	return 0;
>  }

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