Re: [PATCH 2/6] iommu/amd: Clear DTE with update_dte256 in iommu_ignore_device()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Calling `setup_aliases()` before clearing the primary DTE propagates the active DTE to aliases instead of safely clearing them.
- [Low] The variable `dev_data` is used in `iommu_ignore_device()` without being declared or initialized, causing a build failure. Note: I found the fix later in the series in commit 'iommu/amd: Split probe error paths to preserve IRQ remapping'.

Pre-existing issues:
- [High] The patch leaves the hardware torn-write vulnerability unmitigated when `dev_data` is NULL by falling back to unsafe `memset()`.
--

commit 57abff52ae96ef6d8a5ffe516b907b27e8c51040
Author: Pranjal Shrivastava <praan@google.com>
Subject: iommu/amd: Clear DTE with update_dte256 in iommu_ignore_device()

This commit modifies iommu_ignore_device() to use update_dte256() instead
of memset() for clearing the Device Table Entry (DTE) when dev_data is
available. This aims to prevent torn writes caused by the hardware reading
256-bit DTEs atomically.

Link: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -750,7 +750,16 @@ static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
>  	setup_aliases(iommu, dev);

[Severity: High]
Does calling setup_aliases() before clearing the primary DTE cause the
aliases to receive the active DTE?

Looking at setup_aliases(), it invokes clone_aliases() which reads the
current primary DTE and copies it to all alias DTEs. Since this happens
before the primary DTE is cleared below, it appears the aliases remain
active with the uncleared DTE.

>  
>  	pci_seg->rlookup_table[devid] = NULL;
> -	memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
> +
> +	/* Clear DTE if we have a live entry */
> +	if (dev_data) {

[Severity: Low]
Is dev_data declared in this scope? It appears this will cause a compilation
failure since dev_data is evaluated without being declared or initialized in
iommu_ignore_device().

I noticed this build issue is fixed later in the series by commit
'iommu/amd: Split probe error paths to preserve IRQ remapping'.

> +		struct dev_table_entry new = {};
> +
> +		amd_iommu_make_clear_dte(dev_data, &new);
> +		update_dte256(iommu, dev_data, &new);
> +	} else {
> +		memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));

[Severity: High]
This is a pre-existing issue, but does falling back to memset() leave the
hardware torn-write vulnerability unmitigated when dev_data is absent?

If a device lacks dev_data, such as during failed initialization, the BIOS
or ACPI IVHD table may have already established a live DTE. Executing
memset() in this state could still cause a torn read by the active IOMMU
hardware.

> +	}
>  }

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