* Re: [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream
2026-10-20 18:10 [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream Danny Gonzalez
@ 2026-10-20 18:36 ` sashiko-bot
2026-11-05 23:04 ` Bjorn Helgaas
1 sibling, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-20 18:36 UTC (permalink / raw)
To: Danny Gonzalez; +Cc: linux-pci
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream
2026-10-20 18:10 [PATCH] PCI/ATS: Disable ATS when ACS TB is set upstream Danny Gonzalez
2026-10-20 18:36 ` sashiko-bot
@ 2026-11-05 23:04 ` Bjorn Helgaas
2026-11-05 23:15 ` Nicolin Chen
2026-11-06 17:49 ` Jason Gunthorpe
1 sibling, 2 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2026-11-05 23:04 UTC (permalink / raw)
To: Danny Gonzalez
Cc: Bjorn Helgaas, David Decotigny, Sheena Mohan, linux-pci,
linux-kernel, Jason Gunthorpe, Pranjal Shrivastava, Joerg Roedel,
Nicolin Chen, Lu Baolu, Kevin Tian
[+cc other folks interested in ATS]
On Tue, Oct 20, 2026 at 06:10:52PM +0000, Danny Gonzalez wrote:
> Enabling ATS allows an endpoint to cache translations and initiate DMA
> requests with Translated addresses, which an upstream bridge with ACS
> Translation Blocking (PCI_ACS_TB) set will block as ACS Violations.
> Because pci_enable_acs() preserves existing PCI_ACS_CTRL bits (e.g., from
> firmware, a kexec kernel, or "pci=config_acs=") without relaxing them,
> PCI_ACS_TB may remain set on upstream bridges.
>
> Update pci_ats_supported() to walk upstream bridges and return false if
> any bridge has PCI_ACS_TB set, leaving the endpoint to use Untranslated
> addresses.
>
> Tested by booting a machine with an endpoint advertising the ATS Capability
> behind a Root Port with PCI_ACS_TB set; verified ATS is disabled and the
> system boots cleanly.
There was a v2 at
https://lore.kernel.org/all/20261028233651.960835-1-digonzal@google.com,
which added pci_ats_check_acs() in pci_dma_configure(). I thought
that was a non-obvious way to make pci_ats_supported() return "false",
and I proposed doing the checking in pci_ats_supported() directly to
avoid adding the new pci_ats_check_acs() interface.
I'm responding to v1 because it *did* add the checking in
pci_ats_supported(). But sashiko found some issues with it:
https://lore.kernel.org/all/sashiko-outbox-179488@kernel.org.
> Signed-off-by: Danny Gonzalez <digonzal@google.com>
> ---
> drivers/pci/ats.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 9cb23780093d..094f4f825acf 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);
Sashiko noted that if bridge is inaccessible, this read sets ctrl to
~0, so we think PCI_ACS_TB is set. Maybe worth checking for
PCI_POSSIBLE_ERROR() before using ctrl.
> + if (ctrl & PCI_ACS_TB)
> + return bridge;
> + }
> + 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));
I think I suggested it, but now I'm not sure the warning is
worthwhile. I think this might be too far down in the bowels for it
to be useful. And sashiko complained about the warn-once bit too,
which is fair.
> + return false;
> + }
> +
Sashiko also noted problems if pci_ats_supported() returns different
values in:
* intel_iommu_probe_device() vs intel_iommu_release_device()
Different values break rbtree 'info' management: _probe_device()
can put 'info' in the rbtree and _release_device() may fail to
remove it.
I think the fix is for intel_iommu_probe_device() to keep its own
bit in struct device_domain_info to say whether 'info' is in the
rbtree so _release_device() doesn't need pci_ats_supported().
* pci_dev_reset_iommu_prepare() vs pci_dev_reset_iommu_done()
Different values will make gdev->reset_depth, group->recovery_cnt,
group->domain, and gdev->blocked get out of sync.
I think it would be better if _iommu_prepare() and _iommu_done()
themselves tracked what _iommu_done() needs to do, for example
they could do something like this:
int pci_dev_reset_iommu_prepare(struct pci_dev *pdev, bool *restore)
{
if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev)) {
*restore = false;
return 0;
}
*restore = true;
...
}
Then pci_dev_reset_iommu_done() wouldn't need to call
pci_ats_supported() or dev_has_iommu(), and users would look like
this:
int pcie_flr(...)
{
bool restore;
ret = pci_dev_reset_iommu_prepare(dev, &restore);
...
if (restore)
pci_dev_reset_iommu_done(dev);
}
Or I guess we could call pci_dev_reset_iommu_done()
unconditionally and pass it 'restore' and it could test it
internally.
> return true;
> }
> EXPORT_SYMBOL_GPL(pci_ats_supported);
> --
> 2.56.0.218.g1ec904b173-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread