Re: [PATCH v2 18/19] PCI: Replace untrusted state with device trust

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] PCI: broken adversarial trust inheritance in
  pci_set_default_trust_policy()
- [High] PCI: default trust policy overwrites adversarial default on
  removable devices
- [Medium] PCI: pci_enable_acs() and pci_ats_init() break
  disabled-to-full trust policy
- [Medium] PCI: pci_dev_ats_permitted() drops PCI_ACS_TB after full
  authorization
- [Medium] PCI: of_create_pci_dev() skips pci_set_default_trust_policy()
  before ATS init
--

commit 7c55633025e040b3c2b6b2f31a6d205ea496c0f7
Author: Leon Romanovsky <leon@kernel.org>

PCI: Replace untrusted state with device trust

This commit replaces pci_dev::untrusted with the generic device trust policy
and trust level helpers across the PCI and IOMMU subsystems.

> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 9cb23780093dd..c8c96acc26343 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -21,7 +21,7 @@ void pci_ats_init(struct pci_dev *dev)
>  {
>  	int pos;
>  
> -	if (pci_ats_disabled() || dev->untrusted)
> +	if (pci_ats_disabled() || pci_dev_default_is_restricted(dev))
>  		return;
>  
>  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> @@ -40,7 +40,7 @@ void pci_ats_init(struct pci_dev *dev)
>   */
>  bool pci_ats_supported(struct pci_dev *dev)
>  {
> -	if (!dev->ats_cap || dev->untrusted)
> +	if (!dev->ats_cap)
>  		return false;
>  
>  	if (dev->is_virtfn)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7c6f2aba81443..b08adba03f96b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1016,8 +1016,9 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	/* Upstream Forwarding */
>  	caps->ctrl |= (dev->acs_capabilities & PCI_ACS_UF);
>  
> -	/* Enable Translation Blocking for external devices and noats */
> -	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
> +	/* Enable Translation Blocking when ATS is unavailable or restricted. */
> +	if (pci_ats_disabled() || dev->external_facing ||
> +	    !pci_dev_ats_permitted(dev))
>  		caps->ctrl |= (dev->acs_capabilities & PCI_ACS_TB);
>  }

[Severity: Medium]
Does a device that starts with DEVICE_TRUST_POLICY_DISABLED (for example via
device_trust.default=disabled before initrd authorization or a driver trust
rule) keep ATS disabled and PCI_ACS_TB enabled even after it is authorized for
DEVICE_TRUST_FULL?

During pci_device_add(), pci_ats_init() sees pci_dev_default_is_restricted()
return true and skips initializing dev->ats_cap; nothing re-runs
pci_ats_init() when the device later transitions to DEVICE_TRUST_FULL.
Likewise, device_add() -> iommu_probe_device() -> pci_dma_configure() ->
pci_enable_acs() runs while the device is still unbound
(DEVICE_TRUST_DISABLED), so !pci_dev_ats_permitted(dev) is true and
pci_std_enable_acs() (as well as pci_quirk_enable_intel_spt_pch_acs()) sets
PCI_ACS_TB in hardware.  When really_probe() later sets DEVICE_TRUST_FULL and
calls pci_dma_configure() -> pci_enable_acs() a second time, pci_enable_acs()
reads the existing PCI_ACS_CTRL register value into caps.ctrl and only ORs
bits into it, never clearing PCI_ACS_TB.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5b6960bbb35bc..607569c31b5b9 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1738,24 +1738,24 @@ 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 (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 downstream devices behind a removable PCIe switch or SR-IOV VFs on a
removable PF lose their adversarial default policy when the parent's
trust_policy is overridden?

In pci_setup_device(), device_trust_init(&dev->dev) runs on the parent bridge
before child buses are scanned (or before SR-IOV VFs are created).  If the
parent port or PF has its trust_policy changed to disabled or full (for
example via device_trust.default=disabled, a per-device rule, or sysfs),
device_policy_is_adversarial(&parent->dev) is false for its children.  On x86
ACPI systems, arch_pci_dev_is_removable() only returns true for the switch
upstream port directly beneath an integrated USB4/Thunderbolt root port and
returns false for the switch's downstream ports, endpoints below them, and
VFs:

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;

	if (pcie_switch_directly_under(root, pdev))
		return false;
	...
}

Those child devices therefore stay at DEVICE_TRUST_POLICY_DEFAULT and resolve
to DEVICE_TRUST_FULL, bypassing IOMMU DMA domain enforcement, ATS and ACS
restrictions, and SWIOTLB bounce buffering.  Running pci_set_removable(dev)
before pci_set_default_trust_policy(dev) and checking dev_is_removable() would
preserve the hardware removability state across parent policy overrides.

> @@ -2082,7 +2082,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);
>  
>  	/* "Unknown power state" */

[Severity: Medium]
On PowerPC and SPARC systems where of_create_pci_dev() creates PCI devices
directly without calling pci_setup_device(), does pci_ats_init() run before
the device trust policy is initialized?

of_create_pci_dev() calls pci_device_add(), which runs
pci_init_capabilities() -> pci_ats_init() before device_add() calls
device_trust_init(), and never calls pci_set_default_trust_policy().  Because
pci_ats_supported() no longer checks the device state at runtime, dev->ats_cap
is populated while trust_policy is still DEVICE_TRUST_POLICY_DEFAULT, leaving
ATS enabled even when a boot trust policy restricts the device.

> 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;
> +}

[Severity: High]
If a removable PCI device is configured with an explicit "default" trust
policy (via device_trust=, device_trust.driver=, or sysfs trust_policy), does
that overwrite DEVICE_TRUST_POLICY_ADVERSARY with DEVICE_TRUST_POLICY_DEFAULT
and grant the external device DEVICE_TRUST_FULL?

In pci_setup_device(), device_trust_init() runs after
pci_set_default_trust_policy() and overwrites dev->dev.trust_policy with
DEVICE_TRUST_POLICY_DEFAULT when a per-device default rule matches.  Because
pci_dev_default_is_restricted() only checks for DISABLED and ADVERSARY, and
pci_bus_type does not implement .trust_resolve, DEVICE_TRUST_POLICY_DEFAULT
bypasses pci_dev_default_is_restricted() during discovery and resolves to
DEVICE_TRUST_FULL in __device_trust_prepare() at bind time.

> +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]
Can a device discovered with DEVICE_TRUST_POLICY_DISABLED lose PCI_ACS_TB in
hardware after being authorized for DEVICE_TRUST_FULL and undergoing a PCI
reset or D3cold transition?

At discovery, pci_ats_init() skips setting dev->ats_cap and pci_enable_acs()
sets PCI_ACS_TB because !pci_dev_ats_permitted(dev) is true.  Once the device
is authorized and bound with DEVICE_TRUST_FULL, pci_dev_ats_permitted()
returns true even though dev->ats_cap is 0.  Because ACS registers are not
saved in saved_cap_space, a subsequent reset or D3cold resume clears the
hardware register and runs pci_restore_state() -> pci_enable_acs() ->
pci_std_enable_acs(), which sees pci_dev_ats_permitted() return true and
leaves PCI_ACS_TB cleared.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-device-trust-core-v2-0-3b12d9a9f861@nvidia.com?part=18