Re: [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] PCI/ATS: unchecked config read in pci_acs_tb_path_blocked() causes UAF
- [Medium] PCI/ATS: pci_ats_supported() reads ACS TB before pci_enable_acs() runs
--

commit 588c0872c861661efe593d12f223aa11b234bb51
Author: Danny Gonzalez <digonzal@google.com>

PCI/ATS: Disable ATS when ACS TB is set upstream

This commit updates pci_ats_supported() to walk upstream bridges via a new
pci_acs_tb_path_blocked() helper and return false if any upstream bridge has
translation blocking enabled.

> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 9cb23780093dd..094f4f825acf1 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -31,6 +31,24 @@ void pci_ats_init(struct pci_dev *dev)
>  	dev->ats_cap = pos;
>  }
>  
> +static struct pci_dev *pci_acs_tb_path_blocked(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge;
> +
> +	for (bridge = pci_upstream_bridge(dev); bridge;
> +	     bridge = pci_upstream_bridge(bridge)) {
> +		u16 ctrl;
> +
> +		if (!bridge->acs_cap)
> +			continue;
> +		pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +				     &ctrl);
> +		if (ctrl & PCI_ACS_TB)
> +			return bridge;

[Severity: Critical]
Can this unchecked config read in drivers/pci/ats.c:pci_acs_tb_path_blocked()
cause pci_ats_supported() to flip from true to false when an upstream bridge
becomes inaccessible, leading to a use-after-free in iommu->device_rbtree and
leaked reset state?

Prior to this change, pci_ats_supported() only inspected invariant fields on
struct pci_dev (dev->ats_cap and dev->untrusted). With this change, every call
to pci_ats_supported() reads config space on each upstream bridge where
bridge->acs_cap is non-zero.

If an upstream bridge is marked disconnected (for example during surprise
hot-unplug when pciehp_unconfigure_device() or trim_stale_devices() marks the
hierarchy disconnected via pci_dev_set_disconnected() before tearing down
child endpoints) or if the link to the bridge goes down, enters d3cold, or
enters containment, drivers/pci/access.c:pci_read_config_word() sets ctrl to
all-ones (0xffff):

drivers/pci/access.c:pci_read_config_word() {
	if (pci_dev_is_disconnected(dev)) {
		PCI_SET_ERROR_RESPONSE(val);
		return PCIBIOS_DEVICE_NOT_FOUND;
	}
	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
}

Because (0xffff & PCI_ACS_TB) is non-zero, pci_acs_tb_path_blocked() treats
the 0xffff error response as translation blocking being set and returns the
bridge, flipping pci_ats_supported() from true to false (which can also happen
if the bridge's control register is modified at runtime).

In drivers/iommu/intel/iommu.c:intel_iommu_probe_device(), info is inserted
into iommu->device_rbtree when pci_ats_supported(pdev) is true:

drivers/iommu/intel/iommu.c:intel_iommu_probe_device() {
	...
	if (pdev && pci_ats_supported(pdev)) {
		ret = pci_prepare_ats(pdev, VTD_PAGE_SHIFT);
		if (ret)
			goto free;

		ret = device_rbtree_insert(iommu, info);
		if (ret)
			goto free;
	}
	...
}

However, in drivers/iommu/intel/iommu.c:intel_iommu_release_device(), removal
from iommu->device_rbtree is also gated on pci_ats_supported(to_pci_dev(dev))
before info is freed:

drivers/iommu/intel/iommu.c:intel_iommu_release_device() {
	...
	mutex_lock(&iommu->iopf_lock);
	if (dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev)))
		device_rbtree_remove(info);
	mutex_unlock(&iommu->iopf_lock);
	...
	kfree(info);
}

If pci_ats_supported() was true at probe time but returns false during
device teardown, device_rbtree_remove(info) is skipped and info is freed
while still linked in iommu->device_rbtree.

Similarly, drivers/iommu/iommu.c:pci_dev_reset_iommu_prepare() and
drivers/iommu/iommu.c:pci_dev_reset_iommu_done() both check
pci_ats_supported(pdev):

drivers/iommu/iommu.c:pci_dev_reset_iommu_done() {
	...
	if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev))
		return;
	...
}

If an upstream bridge becomes inaccessible during a reset after
pci_dev_reset_iommu_prepare() succeeded, wouldn't pci_dev_reset_iommu_done()
return early without decrementing gdev->reset_depth or group->recovery_cnt or
clearing gdev->blocked, leaving the group permanently attached to
group->blocking_domain?

> +	}
> +	return NULL;
> +}
> +
>  /**
>   * pci_ats_supported - check if the device can use ATS
>   * @dev: the PCI device
> @@ -40,12 +58,22 @@ void pci_ats_init(struct pci_dev *dev)
>   */
>  bool pci_ats_supported(struct pci_dev *dev)
>  {
> +	struct pci_dev *bridge;
> +
>  	if (!dev->ats_cap || dev->untrusted)
>  		return false;
>  
>  	if (dev->is_virtfn)
>  		return pci_ats_supported(pci_physfn(dev));
>  
> +	/* Do not allow ATS if any upstream bridge blocks translation */
> +	bridge = pci_acs_tb_path_blocked(dev);
> +	if (bridge) {
> +		pci_warn_once(dev, "ATS disabled: upstream bridge %s has ACS Translation Blocking enabled\n",
> +			      pci_name(bridge));
> +		return false;
> +	}

[Severity: Medium]
Can drivers/pci/ats.c:pci_ats_supported() be invoked during device enumeration
before pci_enable_acs() has configured upstream bridges, firing a false
warning and consuming the single static __print_once state in pci_warn_once()?

During enumeration in pci_device_add() -> pci_init_capabilities() in
drivers/pci/probe.c, pci_init_reset_methods(dev) runs before
device_add(&dev->dev):

drivers/pci/probe.c:pci_init_capabilities() {
	...
	pci_ats_init(dev);		/* Address Translation Services */
	pci_pri_init(dev);		/* Page Request Interface */
	pci_pasid_init(dev);		/* Process Address Space ID */
	pci_acs_init(dev);		/* Access Control Services */
	...
	pci_init_reset_methods(dev);
}

pci_init_reset_methods(dev) probes reset methods via
drivers/pci/pci.c:pci_reset_bus_function() with PCI_RESET_PROBE, which calls
pci_dev_reset_iommu_prepare(dev) and pci_dev_reset_iommu_done(dev):

drivers/pci/pci.c:pci_reset_bus_function() {
	...
	rc = pci_dev_reset_iommu_prepare(dev);
	if (rc) {
		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", rc);
		return rc;
	}
	...
}

In drivers/iommu/iommu.c:pci_dev_reset_iommu_prepare(), pci_ats_supported(pdev)
is evaluated before dev_has_iommu(&pdev->dev):

drivers/iommu/iommu.c:pci_dev_reset_iommu_prepare() {
	...
	if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev))
		return 0;
	...
}

Since commit c41e2fb67e26b0 ("PCI: Enable ACS after configuring IOMMU for OF
platforms"), pci_acs_init() no longer calls pci_enable_acs(); instead,
pci_enable_acs() is called from drivers/pci/pci-driver.c:pci_dma_configure():

drivers/pci/pci-driver.c:pci_dma_configure() {
	...
	pci_enable_acs(to_pci_dev(dev));
	...
}

On acpi platforms, acpi_init() enumerates devices and runs
pci_init_capabilities() before iommu_subsys_init(), pci_iommu_init(), and
pcie_portdrv_init() run. If firmware left translation blocking set on an
upstream bridge and the user passed pci=config_acs=00xxxx@... on the command
line to clear it, wouldn't pci_ats_supported() read the stale firmware value
during pci_init_reset_methods(), log a false warning even though
pci_enable_acs() will later clear the bit, and exhaust the function-static
pci_warn_once() flag so later warnings on other devices are suppressed?

> +
>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_supported);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020181053.2646782-1-digonzal@google.com?part=1