Re: [PATCH 1/6] iommu/amd: Clear aliases before setting the rlookup_table to NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Moving setup_aliases() before clearing the primary DTE incorrectly copies the active (uncleared) DTE to DMA aliases, failing to clear them.
- [Medium] Reordering setup_aliases() before clearing rlookup_table exposes a missing dev_data error path, causing log spam and aborting alias iteration.
--

commit 3c05610e625a1605bf16151c884edcdce09fdf2d
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Clear aliases before setting the rlookup_table to NULL

In iommu_ignore_device(), clearing the primary devid in the lookup table
before calling setup_aliases() causes rlookup_amd_iommu() to fail for
aliases. This commit moves the setup_aliases() call before clearing the
rlookup_table entry.

Fixes: 99fc4ac3d297 ("iommu/amd: Introduce per PCI segment alias_table")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/
Signed-off-by: Pranjal Shrivastava <praan@google.com>

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index f1333071da103..a94de66a885e7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -745,10 +745,12 @@ static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
>  		return;
>  
>  	devid = PCI_SBDF_TO_DEVID(sbdf);
> -	pci_seg->rlookup_table[devid] = NULL;
> -	memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
>  
> +	/* Clear aliases while rlookup is still valid */
>  	setup_aliases(iommu, dev);

[Severity: High]
Does this copy the active DTE instead of the cleared DTE to the DMA aliases?

By moving the call to setup_aliases() before the memset() that clears the
primary Device Table Entry (DTE), setup_aliases() will read the primary
device's uncleared DTE and write this active DTE to the aliases.

Then memset() clears only the primary DTE, leaving the aliases retaining
their old active translations. Could this lead to unauthorized DMA or hardware
use-after-free if the domain's page tables are subsequently freed?

[Severity: Medium]
Does this reordering also expose a missing dev_data error path when
iommu_ignore_device() is called during early initialization?

If this is called from an early initialization error path before dev_data is
set, dev_data will be NULL. Previously, because rlookup_table[devid] was set
to NULL first, rlookup_amd_iommu() inside clone_alias() would return NULL and
safely exit without further processing.

With this patch, rlookup_table is still valid when setup_aliases() is called,
so clone_alias() proceeds and hits the check:

drivers/iommu/amd/iommu.c:clone_alias() {
    ...
    dev_data = dev_iommu_priv_get(&pdev->dev);
    if (!dev_data) {
        pr_err("%s : Failed to get dev_data for 0x%x\n", __func__, devid);
        ret = -EINVAL;
        goto out;
    }
    ...
}

Returning -EINVAL causes pci_for_each_dma_alias() to immediately abort its
iteration. Can this cause unintended log spam and prevent the remaining
aliases from being processed?

> +
> +	pci_seg->rlookup_table[devid] = NULL;
> +	memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
>  }

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