Re: [PATCH 02/13] PCI: Disable pcie port and access on D3 on specific amd devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] PCI: uninitialized *val in pci_read_config_*() when ignoring D3cold access
- [Critical] PCI: pci_user_read_config_*() and cap walks bypass pci_dev_ignore_access()
- [High] PCI: missing 0x7360 fixup in quirk_disable_pcie_port() for MacBookPro16,4
--

commit f3d5ededdfe7b0bad0fd200b82a9eb2e3e834ed0
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

PCI: Disable pcie port and access on D3 on specific amd devices

This commit adds an early PCI fixup for Apple MacBookPro16,1 and
MacBookPro16,4 systems to disable the PCIe port driver and ignore config
space accesses on upstream bridges of the discrete GPU when those bridges
are in D3cold.

> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 795d81c7a4de4..66829941e8fe2 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -1094,6 +1094,35 @@ static void asus_disable_nvme_d3cold(struct pci_dev *pdev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9a09, asus_disable_nvme_d3cold);
>  
> +static const struct dmi_system_id quirk_disable_pcie_port_dmi_table[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,1"),
> +		},
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro16,4"),
> +		},
> +	},
> +	{}
> +};
> +
> +static void quirk_disable_pcie_port(struct pci_dev *pdev)
> +{
> +	struct pci_dev *upstream = pdev;
> +
> +	if (dmi_check_system(quirk_disable_pcie_port_dmi_table)) {
> +		while ((upstream = pci_upstream_bridge(upstream))) {
> +			upstream->no_pcie_port = 1;
> +			upstream->ignore_access_d3 = 1;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ATI, 0x7340, quirk_disable_pcie_port);

[Severity: High]
While quirk_disable_pcie_port_dmi_table in arch/x86/pci/fixup.c matches both
MacBookPro16,1 and MacBookPro16,4, DECLARE_PCI_FIXUP_EARLY is only registered
for PCI device ID 0x7340 (Navi 14, used in MacBookPro16,1).

MacBookPro16,4 uses the AMD Radeon Pro 5600M (Navi 12, PCI device ID 0x7360).
Without a matching fixup registration for 0x7360, will
quirk_disable_pcie_port() ever run on MacBookPro16,4, or will its upstream
bridges remain unprotected when apple-gmux powers them off?

[ ... ]

> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63ba..5e95130671767 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word);
>  int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;

[Severity: Critical]
When pci_dev_ignore_access(dev) is true for a bridge in PCI_D3cold,
pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword()
in drivers/pci/access.c return PCIBIOS_SUCCESSFUL (0) without initializing
*val.

Many callers pass an uninitialized stack variable and rely on either a
non-zero return value or PCI_SET_ERROR_RESPONSE(val) (0xff / 0xffff /
0xffffffff) when the device cannot be read.

For example, in drivers/pci/pci-sysfs.c, secondary_bus_number_show() and
subordinate_bus_number_show() read world-readable sysfs attributes into
uninitialized stack variables:

drivers/pci/pci-sysfs.c:secondary_bus_number_show() {
	struct pci_dev *pci_dev = to_pci_dev(dev);
	u8 sec_bus;
	int err;

	pci_config_pm_runtime_get(pci_dev);
	err = pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus);
	pci_config_pm_runtime_put(pci_dev);

	if (err)
		return -EINVAL;

	return sysfs_emit(buf, "%u\n", sec_bus);
}

In addition, pci_update_current_state() and pci_power_up() in
drivers/pci/pci.c read PCI_PM_CTRL into an uninitialized stack variable
pmcsr and check PCI_POSSIBLE_ERROR(pmcsr):

drivers/pci/pci.c:pci_update_current_state() {
	...
	} else if (dev->pm_cap) {
		u16 pmcsr;

		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
		if (PCI_POSSIBLE_ERROR(pmcsr)) {
			dev->current_state = PCI_D3cold;
			return;
		}
		dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
	}
}

If pmcsr happens not to be 0xffff on the stack, dev->current_state is
overwritten with D0..D3hot (or set to PCI_D0 in pci_power_up()), which
disarms pci_dev_ignore_access(dev) while the bridge is still powered off.
Similarly, during suspend on the driverless bridge,
pci_pm_suspend_noirq() -> pci_save_state() -> pci_save_pcie_state() ->
pcie_capability_read_word() will see PCIBIOS_SUCCESSFUL with *val == 0 and
overwrite the bridge's saved PCIe capability state with zeros.

Should pci_read_config_byte(), pci_read_config_word(), and
pci_read_config_dword() call PCI_SET_ERROR_RESPONSE(val) and return an error
such as PCIBIOS_DEVICE_NOT_FOUND (matching pci_dev_is_disconnected(dev)), or
otherwise ensure *val is never left uninitialized?

> @@ -590,6 +599,9 @@ EXPORT_SYMBOL(pci_read_config_dword);
>  
>  int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
> @@ -598,6 +610,9 @@ EXPORT_SYMBOL(pci_write_config_byte);
>  
>  int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
> @@ -607,6 +622,9 @@ EXPORT_SYMBOL(pci_write_config_word);
>  int pci_write_config_dword(const struct pci_dev *dev, int where,
>  					 u32 val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);

[Severity: Critical]
Does restricting only pci_read_config_*() and pci_write_config_*() in
drivers/pci/access.c leave other config space access paths unprotected when
the bridge is in PCI_D3cold?

Userspace config reads and writes via sysfs (pci_read_config() and
pci_write_config() in drivers/pci/pci-sysfs.c) and procfs (drivers/pci/proc.c)
call pci_user_read_config_*() and pci_user_write_config_*(), which are
generated by PCI_USER_READ_CONFIG and PCI_USER_WRITE_CONFIG in
drivers/pci/access.c:

drivers/pci/access.c:PCI_USER_READ_CONFIG() {
	...
	raw_spin_lock_irq(&pci_lock);
	if (unlikely(dev->block_cfg_access))
		pci_wait_cfg(dev);
	ret = dev->bus->ops->read(dev->bus, dev->devfn,
				  pos, sizeof(type), &data);
	raw_spin_unlock_irq(&pci_lock);
	...
}

Because dev->no_pcie_port = 1 prevents pcie_portdrv_probe() from binding to
the upstream bridges, runtime PM is not enabled on them and
pci_config_pm_runtime_get() in pci_read_config() is a no-op. Any unprivileged
process (such as lspci or udev) reading /sys/bus/pci/devices/.../config while
the bridge is powered off in PCI_D3cold will invoke dev->bus->ops->read()
directly on the dead bridge.

In addition, capability walks in drivers/pci/pci.c (pci_find_capability() ->
__pci_bus_find_cap_start(), pci_find_next_capability(), and
pci_find_next_ext_capability()) call pci_bus_read_config_*() directly rather
than pci_read_config_*(). Because the bridge has no bound driver (pm == NULL),
during system suspend pci_pm_suspend_noirq() in drivers/pci/pci-driver.c
runs:

  pci_pm_suspend_noirq()
    -> pci_save_state()
      -> pci_save_pcix_state()
        -> pci_find_capability(dev, PCI_CAP_ID_PCIX)
          -> __pci_bus_find_cap_start()
            -> pci_bus_read_config_word(bus, devfn, PCI_STATUS, &status)

Could these paths still access the powered-off bridge and trigger a hardware
crash unless PCI_USER_READ_CONFIG, PCI_USER_WRITE_CONFIG, and the capability
lookup paths (or a lower layer) also check pci_dev_ignore_access(dev)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025040046.1602-1-atharvatiwarilinuxdev@gmail.com?part=2