Re: [PATCH] PCI: pciehp: Detect device replacement during system sleep

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, May 29, 2024 at 04:32:09PM +0200, Lukas Wunner wrote:
> Ricky reports that replacing a device in a hotplug slot during ACPI
> sleep state S3 does not cause re-enumeration on resume, as one would
> expect.  Instead, the new device is treated as if it was the old one.
> 
> There is no bulletproof way to detect device replacement, but as a
> heuristic, check whether the device identity in config space matches
> cached data in struct pci_dev (Vendor ID, Device ID, Class Code,
> Revision ID, Subsystem Vendor ID, Subsystem ID).  Additionally, cache
> and compare the Device Serial Number (PCIe r6.2 sec 7.9.3).  If a
> mismatch is detected, mark the old device disconnected (to prevent its
> driver from accessing the new device) and synthesize a Presence Detect
> Changed event.
> 
> The device identity in config space which is compared here is the same
> as the one included in the signed Subject Alternative Name per PCIe r6.1
> sec 6.31.3.  Thus, the present commit prevents attacks where a valid
> device is replaced with a malicious device during system sleep and the
> valid device's driver obliviously accesses the malicious device.
> 
> This is about as much as can be done at the PCI layer.  Drivers may have
> additional ways to identify devices (such as reading a WWID from some
> register) and may trigger re-enumeration when detecting an identity
> change on resume.
> 
> Reported-by: Ricky Wu <ricky_wu@realtek.com>
> Closes: https://lore.kernel.org/r/a608b5930d0a48f092f717c0e137454b@realtek.com
> Tested-by: Ricky Wu <ricky_wu@realtek.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/hotplug for v6.11, thanks, Lukas!

> ---
>  drivers/pci/hotplug/pciehp.h      |  4 ++++
>  drivers/pci/hotplug/pciehp_core.c | 42 ++++++++++++++++++++++++++++++++++++++-
>  drivers/pci/hotplug/pciehp_hpc.c  |  5 +++++
>  drivers/pci/hotplug/pciehp_pci.c  |  4 ++++
>  4 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index e0a614a..273dd8c 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -46,6 +46,9 @@
>  /**
>   * struct controller - PCIe hotplug controller
>   * @pcie: pointer to the controller's PCIe port service device
> + * @dsn: cached copy of Device Serial Number of Function 0 in the hotplug slot
> + *	(PCIe r6.2 sec 7.9.3); used to determine whether a hotplugged device
> + *	was replaced with a different one during system sleep
>   * @slot_cap: cached copy of the Slot Capabilities register
>   * @inband_presence_disabled: In-Band Presence Detect Disable supported by
>   *	controller and disabled per spec recommendation (PCIe r5.0, appendix I
> @@ -87,6 +90,7 @@
>   */
>  struct controller {
>  	struct pcie_device *pcie;
> +	u64 dsn;
>  
>  	u32 slot_cap;				/* capabilities and quirks */
>  	unsigned int inband_presence_disabled:1;
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index ddd55ad..ff458e6 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -284,6 +284,32 @@ static int pciehp_suspend(struct pcie_device *dev)
>  	return 0;
>  }
>  
> +static bool pciehp_device_replaced(struct controller *ctrl)
> +{
> +	struct pci_dev *pdev __free(pci_dev_put);
> +	u32 reg;
> +
> +	pdev = pci_get_slot(ctrl->pcie->port->subordinate, PCI_DEVFN(0, 0));
> +	if (!pdev)
> +		return true;
> +
> +	if (pci_read_config_dword(pdev, PCI_VENDOR_ID, &reg) ||
> +	    reg != (pdev->vendor | (pdev->device << 16)) ||
> +	    pci_read_config_dword(pdev, PCI_CLASS_REVISION, &reg) ||
> +	    reg != (pdev->revision | (pdev->class << 8)))
> +		return true;
> +
> +	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
> +	    (pci_read_config_dword(pdev, PCI_SUBSYSTEM_VENDOR_ID, &reg) ||
> +	     reg != (pdev->subsystem_vendor | (pdev->subsystem_device << 16))))
> +		return true;
> +
> +	if (pci_get_dsn(pdev) != ctrl->dsn)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int pciehp_resume_noirq(struct pcie_device *dev)
>  {
>  	struct controller *ctrl = get_service_data(dev);
> @@ -293,9 +319,23 @@ static int pciehp_resume_noirq(struct pcie_device *dev)
>  	ctrl->cmd_busy = true;
>  
>  	/* clear spurious events from rediscovery of inserted card */
> -	if (ctrl->state == ON_STATE || ctrl->state == BLINKINGOFF_STATE)
> +	if (ctrl->state == ON_STATE || ctrl->state == BLINKINGOFF_STATE) {
>  		pcie_clear_hotplug_events(ctrl);
>  
> +		/*
> +		 * If hotplugged device was replaced with a different one
> +		 * during system sleep, mark the old device disconnected
> +		 * (to prevent its driver from accessing the new device)
> +		 * and synthesize a Presence Detect Changed event.
> +		 */
> +		if (pciehp_device_replaced(ctrl)) {
> +			ctrl_dbg(ctrl, "device replaced during system sleep\n");
> +			pci_walk_bus(ctrl->pcie->port->subordinate,
> +				     pci_dev_set_disconnected, NULL);
> +			pciehp_request(ctrl, PCI_EXP_SLTSTA_PDC);
> +		}
> +	}
> +
>  	return 0;
>  }
>  #endif
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index b1d0a1b3..061f01f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -1055,6 +1055,11 @@ struct controller *pcie_init(struct pcie_device *dev)
>  		}
>  	}
>  
> +	pdev = pci_get_slot(subordinate, PCI_DEVFN(0, 0));
> +	if (pdev)
> +		ctrl->dsn = pci_get_dsn(pdev);
> +	pci_dev_put(pdev);
> +
>  	return ctrl;
>  }
>  
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index ad12515..65e50be 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -72,6 +72,10 @@ int pciehp_configure_device(struct controller *ctrl)
>  	pci_bus_add_devices(parent);
>  	down_read_nested(&ctrl->reset_lock, ctrl->depth);
>  
> +	dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
> +	ctrl->dsn = pci_get_dsn(dev);
> +	pci_dev_put(dev);
> +
>   out:
>  	pci_unlock_rescan_remove();
>  	return ret;
> -- 
> 2.43.0
>