Re: [PATCH v9 14/25] iommu/arm-smmu-v3-kvm: Shadow the command queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iommu/arm-smmu-v3-kvm: wrong WARN_ON() causes hyp panic on invalid host MMIO
- [Medium] iommu/arm-smmu-v3-kvm: WARN_ON used to validate host input in SMMU emulation
--

commit bf6dcb76ab39be1d82600f47f81d772f4c08b9a6
Author: Mostafa Saleh <smostafa@google.com>

iommu/arm-smmu-v3-kvm: Shadow the command queue

This patch allocates a shadow command queue per SMMU to be used by the
hypervisor at boot. The host command queue is pinned in a shared state
when enabled to avoid donation issues and prevent untrusted host access
from reaching the hypervisor state.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c

[ ... ]

> @@ -132,6 +225,23 @@ static int smmu_init(void)
>  	return ret;
>  }
>  
> +static void smmu_emulate_cmdq_enable(struct hyp_arm_smmu_v3_device *smmu)
> +{
> +	u32 shift = smmu->cmdq_host.q_base & Q_BASE_LOG2SIZE;
> +
> +	smmu->cmdq_host.llq.max_n_shift = min(shift, smmu->cmdq_max_shift);
> +	smmu->cmdq_host.base_dma = smmu->cmdq_host.q_base & Q_BASE_ADDR_MASK;
> +	smmu->cmdq_host.base_dma &= ~(cmdq_size(&smmu->cmdq_host) - 1);
> +	WARN_ON(smmu_share_pages(smmu->cmdq_host.base_dma,
> +				 cmdq_size(&smmu->cmdq_host)));

[Severity: Medium]
Will this cause a fatal hypervisor panic if the host provides an invalid
physical address or an unshareable memory range?

At EL2, the WARN_ON() macro behaves as a fatal exception. If the host
configures the emulated SMMU with an invalid address, smmu_share_pages()
can fail and return an error. Wrapping this fallible, host-influenced
operation in WARN_ON() will crash the hypervisor. Could this path safely
ignore the invalid configuration or return an error instead?

> +}
> +
> +static void smmu_emulate_cmdq_disable(struct hyp_arm_smmu_v3_device *smmu)
> +{
> +	WARN_ON(smmu_unshare_pages(smmu->cmdq_host.base_dma,
> +				   cmdq_size(&smmu->cmdq_host)));

[Severity: Medium]
Does this also have the same issue with WARN_ON() panicking the hypervisor
if the host-provided memory range fails to unshare?

> +}
> +

[ ... ]

> @@ -158,6 +268,14 @@ static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
>  		break;
>  	/* Passthrough the register access for bisectability, handled later */
>  	case ARM_SMMU_CMDQ_BASE:
> +		if (is_write) {
> +			/* Not allowed by the architecture */
> +			if (is_cmdq_enabled(smmu))
> +				break;

[Severity: Medium]
If the host kernel makes an invalid MMIO write to ARM_SMMU_CMDQ_BASE while
the CMDQ is enabled, the switch statement breaks and leaves the mask set
to 0.

Later in smmu_dabt_device(), there is a pre-existing check:

	if (WARN_ON(!mask))
		goto out_ret;

Since WARN_ON() is fatal at EL2, will this turn an invalid host MMIO access
into a hypervisor panic? It seems like the access should be gracefully
ignored or rejected without triggering a fatal assertion.

> +			smmu->cmdq_host.q_base = val;
> +		}
> +		mask = read_write;
> +		break;

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