Re: [RFC PATCH v3 6/6] iommu/amd: Provide kernel command line option to enable GAPPI

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unconditional enablement of GA log on overflow in kexec/kdump environments with GAPPI leads to critical DMA memory corruption.

Pre-existing issues:
- [High] Lockless read-modify-write of the IOMMU MMIO control register causes a race condition that can permanently disable IOMMU interrupts or GAPPI.
--

--- Patch [6]: [RFC PATCH v3 6/6] iommu/amd: Provide kernel command line option to enable GAPPI ---
commit 38ebc4ddc54202efd2ae74234b7ee4169af3e39a
Author: Sairaj Kodilkar <sarunkod@amd.com>

iommu/amd: Provide kernel command line option to enable GAPPI

This commit adds a kernel parameter to enable GAPPI for AMD IOMMUs when
the GAPPISup extended feature bit is set. It also modifies initialization
code to handle GAPPI alongside the existing GALOG feature.

> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 2e1889f8a9e42..7d2b1c24f6ecc 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c

[ ... ]

> @@ -2999,6 +3002,13 @@ static void enable_iommus_vapic(void)
>  	struct amd_iommu *iommu;
>  
>  	for_each_iommu(iommu) {
> +		/*
> +		 * Disable GAPPI, do not check amd_iommu_gappi as it may be
> +		 * false in new kexec kernel even though previous kernel has
> +		 * enabled it.
> +		 */
> +		iommu_feature_disable(iommu, CONTROL_GAPPI_EN);

[Severity: High]
This is a pre-existing issue, but does this unlocked modification of the
IOMMU MMIO control register cause a race condition that could disable
interrupts or other features?

The iommu_feature_disable() function performs a read-modify-write without
holding iommu->lock:

drivers/iommu/amd/init.c:iommu_feature_set() {
    ctrl = readq(iommu->mmio_base +  MMIO_CONTROL_OFFSET);
    ctrl &= ~((u64)mask << shift);
    ctrl |= ((u64)val << shift) & ((u64)mask << shift);
    writeq(ctrl, iommu->mmio_base +  MMIO_CONTROL_OFFSET);
}

During initialization, the IRQ thread could be running concurrently. If a log
overflow occurs, the IRQ thread calls amd_iommu_restart_log() which also
invokes iommu_feature_disable().

Since this read-modify-write isn't synchronized, could concurrent
modifications silently overwrite each other?

> +
>  		/*
>  		 * Disable GALog if already running. It could have been enabled

[ ... ]

> @@ -3038,10 +3048,19 @@ static void enable_iommus_vapic(void)
>  		return;
>  	}
>  
> +	if (amd_iommu_gappi &&
> +	    !(check_feature(FEATURE_GAPPI) &&
> +	      AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))) {
> +		pr_warn("GAPPI is not supported.\n");
> +		amd_iommu_gappi = false;
> +	}
> +
>  	/* Enabling GAM and SNPAVIC support */
>  	for_each_iommu(iommu) {
> -		if (iommu_init_ga_log(iommu) ||
> -		    iommu_ga_log_enable(iommu))
> +		if (amd_iommu_gappi)
> +			iommu_feature_enable(iommu, CONTROL_GAPPI_EN);
> +		else if (iommu_init_ga_log(iommu) ||
> +			 iommu_ga_log_enable(iommu))
>  			return;
>  

[Severity: Critical]
If the system is booted into a kdump kernel with GAPPI enabled, could this
lead to the IOMMU writing to stale physical memory from the previous kernel?

When GAPPI is used, iommu_init_ga_log() is skipped, meaning the ga_log buffer
is not allocated. However, if the previous kernel crashed with a pending GA
log overflow, the hardware's overflow status bit remains set.

Once shared IOMMU interrupts are enabled, the pending overflow status triggers
the interrupt thread:

drivers/iommu/amd/iommu.c:amd_iommu_int_thread_galog() {
    amd_iommu_handle_irq(data, "GA", MMIO_STATUS_GALOG_INT_MASK,
                         MMIO_STATUS_GALOG_OVERFLOW_MASK,
                         iommu_poll_ga_log, amd_iommu_restart_ga_log);
}

This calls amd_iommu_restart_ga_log() which unconditionally enables
CONTROL_GALOG_EN:

drivers/iommu/amd/init.c:amd_iommu_restart_ga_log() {
    amd_iommu_restart_log(iommu, "GA", CONTROL_GAINT_EN,
                          CONTROL_GALOG_EN, MMIO_STATUS_GALOG_RUN_MASK,
                          MMIO_STATUS_GALOG_OVERFLOW_MASK);
}

Since CONTROL_GALOG_EN is re-enabled without checking if GAPPI is active or
if the log memory was allocated, will the IOMMU resume writing GA log
entries to the uninitialized physical address left by the crashed kernel?

>  		iommu_feature_enable(iommu, CONTROL_GAM_EN);
>  		if (amd_iommu_snp_en)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713105033.15405-1-sarunkod@amd.com?part=6