Re: [PATCH 4/6] iommu/amd: Fix Use-After-Free in non-fatal probe error path

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The local variable `dev_data` is used uninitialized on the `iommu_init_device` error path, leading to arbitrary memory operations.
- [Critical] Calling `amd_iommu_make_clear_dte` during device probe triggers a NULL pointer dereference because `dev->iommu->iommu_dev` is not yet set.

Pre-existing issues:
- [High] Uncleared active DTE is copied to DMA aliases in `iommu_ignore_device()`, bypassing IOMMU isolation.
--

commit 9fad373a2e7ad46c5f5a781a35dd6f77a626cb3b
Author: Pranjal Shrivastava <praan@google.com>

iommu/amd: Fix Use-After-Free in non-fatal probe error path

When amd_iommu_probe_device() fails for optional features, the dev_data
remains globally linked with a pointer to the struct device. This patch
introduces amd_iommu_clear_device_dte() to clear the HW state and sets
dev_data->dev = NULL on the non-fatal error path to prevent a UAF.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260529153216.2AD1E1F00899@smtp.kernel.org/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index c3b3750d4a22d..4ef6024c5a4ea 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -734,6 +734,15 @@ static int iommu_init_device(struct amd_iommu *iommu, struct device *dev)
>  	return 0;
>  }
>  
> +static void amd_iommu_clear_device_dte(struct amd_iommu *iommu,
> +				       struct iommu_dev_data *dev_data)
> +{
> +	struct dev_table_entry new = {};
> +
> +	amd_iommu_make_clear_dte(dev_data, &new);

[Severity: Critical]
Does calling amd_iommu_make_clear_dte() during device probe trigger a null
pointer dereference?

If amd_iommu_make_clear_dte() is called during the probe error path, it
attempts to fetch the IOMMU via:

drivers/iommu/amd/amd_iommu.h:amd_iommu_make_clear_dte() {
    ...
    struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
    ...
}

This internally accesses dev->iommu->iommu_dev. However, because execution
occurs during the ops->probe_device() phase, the IOMMU core has not yet
populated this pointer. This causes __iommu_get_iommu_dev() to return null,
leading to a null pointer dereference when accessing iommu->pci_seg later in
the function.

Could we pass the explicitly provided iommu pointer directly to
amd_iommu_make_clear_dte() instead of fetching it again?

> +	update_dte256(iommu, dev_data, &new);
> +}
> +
>  static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
>  {
>  	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
> @@ -753,14 +762,10 @@ static void iommu_ignore_device(struct amd_iommu *iommu, struct device *dev)
>  	pci_seg->rlookup_table[devid] = NULL;
>  
>  	/* Clear DTE if we have a live entry */

[Severity: High]
This is a pre-existing issue, but does iommu_ignore_device() copy an
uncleared active DTE to DMA aliases?

Just before clearing the DTE, setup_aliases() is called:

drivers/iommu/amd/iommu.c:iommu_ignore_device() {
    ...
	/* Clear aliases while rlookup is still valid */
	setup_aliases(iommu, dev);

	pci_seg->rlookup_table[devid] = NULL;

	/* Clear DTE if we have a live entry */
    ...
}

Because setup_aliases() is called before clearing the primary device's DTE,
clone_alias() will read the primary device's active, uncleared DTE and write
it to the alias's DTE. When the primary device's DTE is eventually cleared,
the aliases remain fully active and capable of unauthorized DMA, which bypasses
IOMMU isolation for devices with aliases.

> -	if (dev_data) {
> -		struct dev_table_entry new = {};
> -
> -		amd_iommu_make_clear_dte(dev_data, &new);
> -		update_dte256(iommu, dev_data, &new);
> -	} else {
> +	if (dev_data)
> +		amd_iommu_clear_device_dte(iommu, dev_data);
> +	else
>  		memset(&dev_table[devid], 0, sizeof(struct dev_table_entry));
> -	}
>  }
>  
>  
> @@ -2517,6 +2522,11 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>  err_deinit:
>  	iommu_ignore_device(iommu, dev);
>  out_err:
> +	if (dev_data) {

[Severity: Critical]
Is it possible that dev_data is used uninitialized on the iommu_init_device()
error path?

If iommu_init_device() fails, the code jumps to the err_deinit label and falls
through to out_err:

drivers/iommu/amd/iommu.c:amd_iommu_probe_device() {
    ...
	struct iommu_dev_data *dev_data;
    ...
	ret = iommu_init_device(iommu, dev);
	if (ret) {
		dev_err(dev, "Failed to initialize - trying to proceed anyway\n");
		iommu_dev = ERR_PTR(ret);
		goto err_deinit;
	}
    ...
err_deinit:
	iommu_ignore_device(iommu, dev);
out_err:
	if (dev_data) {
		amd_iommu_clear_device_dte(iommu, dev_data);
    ...
}

Because dev_data is declared without initialization and isn't assigned until
later in the function, it will contain stack garbage when the error path is
taken. This could result in passing a garbage pointer to
amd_iommu_clear_device_dte(), leading to arbitrary memory operations.

> +		amd_iommu_clear_device_dte(iommu, dev_data);
> +		dev_data->dev = NULL;
> +	}
> +
>  	return iommu_dev;
>  }

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