Re: [PATCH v2 3/5] iommu/amd: Split probe error paths to preserve IRQ remapping

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

New issues:
- [High] Skipping iommu_disable_device_dma() on probe configuration failures leaves the device's DTE dangerously active, leading to memory corruption or IOMMU faults.

Pre-existing issues:
- [High] A hardware-triggered Peripheral Page Request (PPR) can cause a NULL pointer dereference if the device's IOMMU probe previously failed or was aborted.
--

--- Patch [3]: [PATCH v2 3/5] iommu/amd: Split probe error paths to preserve IRQ remapping ---
commit e8c6c05e87391c978351a97a7ecc5fb4ef5a2b22
Author: Pranjal Shrivastava <[email protected]>

iommu/amd: Split probe error paths to preserve IRQ remapping

This commit splits the error paths in amd_iommu_probe_device() to skip
calling iommu_disable_device_dma() on configuration failures. This aims to
preserve the rlookup_table entry required for IRQ remapping.

Closes: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 91d551f29ed12..9ad3412f0ade8 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c

[ ... ]

> @@ -2564,6 +2563,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>  		goto out_err;
>  	}

[Severity: High]
Does skipping iommu_disable_device_dma() on probe configuration failures
leave the device's Device Table Entry (DTE) dangerously active? 

If the DTE is not properly cleared (e.g., clearing the TV and V bits), could
the AMD hardware continue to intercept DMA using stale or uninitialized state?
This might lead to memory corruption if a kexec environment leaves valid page
tables active, or IOMMU target aborts if a normal boot leaves the page table
root at 0, breaking direct DMA fallback for the device.

>  
> +	return iommu_dev;
> +
> +err_deinit:
> +	iommu_disable_device_dma(iommu, dev);
>  out_err:

[Severity: High]
This is a pre-existing issue, but does leaving the DTE active on probe
failures expose a NULL pointer dereference in the hardware PPR handler?

If the hardware generates a Peripheral Page Request (PPR) because the old
kexec PPR configuration was left active, the interrupt handler is called:

drivers/iommu/amd/ppr.c:iommu_call_iopf_notifier() {
    ...
    dev_data = dev_iommu_priv_get(&pdev->dev);
    if (event.fault.prm.pasid == 0 ||
        event.fault.prm.pasid >= dev_data->max_pasids) {
    ...
}

Since the IOMMU core frees dev->iommu on probe failure, dev_data would be
NULL. Won't unconditionally reading dev_data->max_pasids cause a kernel panic
here?

>  	return iommu_dev;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3