Re: [PATCH 17/17] PCI: Replace untrusted state with device trust

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] PCI: default trust policy overwrites adversarial default in
  pci_setup_device()
- [High] PCI: broken adversarial trust inheritance in
  pci_set_default_trust_policy()
- [Medium] PCI: pci_dev_ats_permitted() drops PCI_ACS_TB after full
  authorization
--

commit 4f93e031c1b1cdb8175550bd66ba1ab96c7a1e00
Author: Leon Romanovsky <leon@kernel.org>

PCI: Replace untrusted state with device trust

This commit replaces the PCI-specific untrusted flag in struct pci_dev with
the generic device trust policy and trust level infrastructure, defaulting
removable PCI devices to DEVICE_TRUST_POLICY_ADVERSARY and updating IOMMU,
ATS, and ACS callers accordingly.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5b6960bbb35bc..139e6f7b4e022 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1738,24 +1738,27 @@ static void set_pcie_cxl(struct pci_dev *dev)
>  
>  }
>  
> -static void set_pcie_untrusted(struct pci_dev *dev)
> +static void pci_set_default_trust_policy(struct pci_dev *dev)
>  {
> -	struct pci_dev *parent = pci_upstream_bridge(dev);
> +	struct pci_dev *parent = pci_physfn(dev);
> +
> +	if (dev->dev.trust_policy != DEVICE_TRUST_POLICY_DEFAULT)
> +		return;
> +
> +	if (parent == dev)
> +		parent = pci_upstream_bridge(dev);
>  
>  	if (!parent)
>  		return;
> -	/*
> -	 * If the upstream bridge is untrusted we treat this device as
> -	 * untrusted as well.
> -	 */
> -	if (parent->untrusted) {
> -		dev->untrusted = true;
> +
> +	if (device_policy_is_adversarial(&parent->dev)) {
> +		dev->dev.trust_policy = DEVICE_TRUST_POLICY_ADVERSARY;
>  		return;
>  	}
>  
>  	if (arch_pci_dev_is_removable(dev)) {
> -		pci_dbg(dev, "marking as untrusted\n");
> -		dev->untrusted = true;
> +		pci_dbg(dev, "setting adversarial trust policy\n");
> +		dev->dev.trust_policy = DEVICE_TRUST_POLICY_ADVERSARY;
>  	}
>  }

[Severity: High]
Can checking device_policy_is_adversarial(&parent->dev) in
pci_set_default_trust_policy() fail to propagate the adversarial default to
downstream ports and endpoints when the upstream bridge's trust_policy has
been overridden?

On x86 ACPI systems, for a PCIe switch connected to an integrated
USB4/Thunderbolt Root Port where root->external_facing and
pcie_has_usb4_host_interface(root) are true, arch_pci_dev_is_removable() in
arch/x86/pci/acpi.c returns true only for the Switch Upstream Port directly
under the Root Port:

arch/x86/pci/acpi.c:arch_pci_dev_is_removable() {
	...
	/* Anything directly behind a "usb4-host-interface" is tunneled */
	if (pcie_has_usb4_host_interface(parent))
		return true;

	/*
	 * Check if this is a discrete Thunderbolt/USB4 controller that is
	 * directly behind the non-USB4 PCIe Root Port marked as
	 * "ExternalFacingPort". Those are not behind a PCIe tunnel.
	 */
	if (pcie_switch_directly_under(root, pdev))
		return false;
	...
}

For the Switch Downstream Ports and Endpoints on that switch,
pcie_has_usb4_host_interface(parent) is false and
pcie_switch_directly_under(root, pdev) returns true, so
arch_pci_dev_is_removable() returns false and relies on inheritance from the
parent bridge.

Because pci_setup_device() calls device_trust_init(&dev->dev) on the parent
bridge before subordinate buses are scanned (or userspace updates the bridge's
trust_policy via sysfs before hotplugging a downstream device), overriding the
Switch Upstream Port's trust_policy to disabled or full changes
parent->dev.trust_policy away from DEVICE_TRUST_POLICY_ADVERSARY.

When child devices are subsequently set up, both
device_policy_is_adversarial(&parent->dev) and arch_pci_dev_is_removable(dev)
return false, leaving removable external devices at
DEVICE_TRUST_POLICY_DEFAULT (which resolves to DEVICE_TRUST_FULL) and
bypassing IOMMU DMA domain enforcement, ATS restrictions, ACS Translation
Blocking, and SWIOTLB bounce buffering.

>  
> @@ -2082,7 +2085,7 @@ int pci_setup_device(struct pci_dev *dev)
>  
>  	set_pcie_cxl(dev);
>  
> -	set_pcie_untrusted(dev);
> +	pci_set_default_trust_policy(dev);
>  	device_trust_init(&dev->dev);

[Severity: High]
What happens if a removable PCI device is configured with an explicit default
policy via device_trust=, device_trust.driver=, or sysfs trust_policy?

In pci_setup_device(), pci_set_default_trust_policy(dev) runs on a freshly
zero-allocated struct pci_dev before device_trust_init(&dev->dev). Because
dev->dev.trust_policy is always DEVICE_TRUST_POLICY_DEFAULT at that point, the
early return in pci_set_default_trust_policy() is never taken, and a removable
device is set to DEVICE_TRUST_POLICY_ADVERSARY.

Immediately afterward, device_trust_init(&dev->dev) invokes
device_trust_apply_boot_policy(). If the kernel command line contains a
per-device default rule such as:

  device_trust.default=disabled device_trust=pci:<bdf>=default

or if default is later selected via device_trust.driver= or by writing default
to sysfs trust_policy before binding, dev->dev.trust_policy is set to
DEVICE_TRUST_POLICY_DEFAULT.

Because pci_set_default_trust_policy() is never called again and pci_bus_type
does not implement bus_type->trust_resolve():
1. pci_dev_default_is_restricted() returns false for
   DEVICE_TRUST_POLICY_DEFAULT, allowing identity IOMMU domains in
   iommu_get_default_domain_type() and amd_iommu_def_domain_type(), IOMMU
   bypass quirks in risky_device(), ATS capability registration in
   pci_ats_init(), and omission of PCI_ACS_TB in pci_enable_acs().
2. At driver bind time, device_trust_prepare() calls
   device_trust_resolve_default(), which resolves DEVICE_TRUST_POLICY_DEFAULT
   to DEVICE_TRUST_FULL instead of DEVICE_TRUST_ADVERSARY, so
   device_is_adversarial() returns false and SWIOTLB bounce buffering in
   drivers/iommu/dma-iommu.c is skipped.

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index aa3765a20a96c..087c63b177c85 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
[ ... ]
> @@ -616,6 +608,28 @@ static inline struct pci_dev *to_pci_dev(const struct device *dev)
>  {
>  	return container_of(dev, struct pci_dev, dev);
>  }
> +
> +static inline bool
> +pci_dev_default_is_restricted(const struct pci_dev *pdev)
> +{
> +	enum device_trust_policy policy = READ_ONCE(pdev->dev.trust_policy);
> +
> +	return policy == DEVICE_TRUST_POLICY_DISABLED ||
> +	       policy == DEVICE_TRUST_POLICY_ADVERSARY;
> +}
> +
> +static inline bool pci_dev_ats_permitted(const struct pci_dev *pdev)
> +{
> +	enum device_trust_level level = device_get_trust_level(&pdev->dev);
> +	enum device_trust_policy policy = READ_ONCE(pdev->dev.trust_policy);
> +
> +	if (level == DEVICE_TRUST_DISABLED)
> +		return policy != DEVICE_TRUST_POLICY_ADVERSARY &&
> +		       policy != DEVICE_TRUST_POLICY_DISABLED;
> +
> +	return level == DEVICE_TRUST_FULL;
> +}

[Severity: Medium]
Won't a device discovered with DEVICE_TRUST_POLICY_DISABLED lose PCI_ACS_TB
after being authorized as full and undergoing a reset or D3cold transition?

When a PCIe port or device is discovered with DEVICE_TRUST_POLICY_DISABLED
(for example via device_trust.default=disabled), pci_ats_init() sees
pci_dev_default_is_restricted(dev) == true and permanently skips recording
dev->ats_cap, while initial pci_enable_acs() sees
!pci_dev_ats_permitted(dev) == true and enables PCI_ACS_TB in
pci_std_enable_acs() or pci_quirk_enable_intel_spt_pch_acs().

If userspace in initrd later sets trust_policy to full and binds a driver,
dev->dev.trust_level becomes DEVICE_TRUST_FULL and pci_dev_ats_permitted(dev)
returns true even though dev->ats_cap is still 0.

Because PCI_EXT_CAP_ID_ACS is not saved in saved_cap_space, a subsequent PCI
reset or D3cold-to-D0 resume resets the hardware ACS control register and runs:

  pci_restore_state()
    -> pci_enable_acs()
      -> pci_std_enable_acs() / pci_quirk_enable_intel_spt_pch_acs()

At that point pci_dev_ats_permitted(dev) returns true, so PCI_ACS_TB is no
longer set even though ATS was never initialized for the device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-device-trust-core-v1-0-1e536825b4eb@nvidia.com?part=17