Re: [PATCH RFC 5/7] i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Dereference of unvalidated `drvdata` in suspend path leads to NULL pointer crash if child driver is unbound.
--

commit 1a652f2d9252aa55258e44d095e09f412a966337
Author: Adrian Hunter <[email protected]>

i3c: mipi-i3c-hci-pci: Propagate I3C wakeup requirements to PCI
    
Keep the PCI wakeup state aligned with the wakeup requirements of the devices
served by the controller(s). It checks whether any HCI instance has a
wakeup-enabled I3C device and enables wakeup for the PCI function.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> index 5a9e2a43eff8d..2b3bf6fa74f29 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/mipi-i3c-hci-pci.c
[ ... ]
> @@ -272,6 +274,13 @@ static bool mipi_i3c_hci_pci_is_mfd(struct device *dev)
>  	return dev_is_platform(dev) && mfd_get_cell(to_platform_device(dev));
>  }
>  
> +static bool mipi_i3c_hci_pci_any_wakeup_enabled(struct device *dev)
> +{
> +	struct i3c_hci *hci = dev_get_drvdata(dev);
> +
> +	return i3c_master_any_wakeup_enabled(&hci->master);
> +}
> +

[Severity: High]
This is a pre-existing issue, but does this code lead to a NULL pointer
dereference if the child driver is unbound?

If a privileged user unbinds the mipi_i3c_hci driver from a child platform
device via sysfs, drvdata is cleared to NULL.

Because the driver's remove callback does not disable the hardware controller
or clear the HC_CONTROL_BUS_ENABLE register bit, 
mipi_i3c_hci_pci_is_operational() can still return true for this device
during a subsequent system suspend.

The PCI suspend instance iterator then passes the potentially unbound device
into this new function, which fetches the NULL drvdata and blindly 
dereferences it.

The pre-existing i3c_hci_rpm_suspend() function called earlier in the same
suspend path appears to suffer from the exact same problem:

core.c:i3c_hci_rpm_suspend() {
	struct i3c_hci *hci = dev_get_drvdata(dev);

	/* Fall back to software reset to disable the bus */
	if (i3c_hci_bus_disable(hci))
        ...
}

Should the suspend path validate that drvdata is non-NULL or that the driver
is actually bound before proceeding with these suspend operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c