Re: [PATCH v8 16/25] iommu/arm-smmu-v3-kvm: Emulate CMDQ for host

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm-smmu-v3-kvm: Incomplete CMD_SYNC filtering allows arbitrary MSI DMA writes
- [Medium] arm-smmu-v3-kvm: Data races on concurrent MMIO writes corrupt hypervisor state
- [Medium] arm-smmu-v3-kvm: Misuse of WARN_ON() for untrusted host inputs panics hypervisor
- [Low] arm-smmu-v3-kvm: Unnecessary wfe() sleep when waiting for CMDQ emptiness causes CPU stalls
--

commit 59e5e956f9f689e4dbd688035f21063722fbd640
Author: Mostafa Saleh <smostafa@google.com>
Subject: iommu/arm-smmu-v3-kvm: Emulate CMDQ for host

This commit adds emulation for the host command queue in the ARM SMMUv3
hypervisor. It prevents direct access to the command queue from the host,
intercepting writes to CMDQ_PROD and CMDQ_CONS to sanitize commands and
manage the queue lifecycle.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> index 38b8ecc5cc10c..8c67e348f4a30 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
[ ... ]
> @@ -340,6 +339,105 @@ static int smmu_init(void)
>  	return ret;
>  }
>  
> +static bool smmu_filter_command(struct hyp_arm_smmu_v3_device *smmu, u64 *command)
> +{
> +	u64 type = FIELD_GET(CMDQ_0_OP, command[0]);
> +
> +	switch (type) {
[ ... ]
> +	case CMDQ_OP_CMD_SYNC:
> +		if (FIELD_GET(CMDQ_SYNC_0_CS, command[0]) == CMDQ_SYNC_0_CS_IRQ) {
> +			/* Do not allow MSI */
> +			command[0] &= ~CMDQ_SYNC_0_CS;
> +			command[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
> +			command[1] &= ~CMDQ_SYNC_1_MSIADDR_MASK;
> +		}
> +		break;

[Severity: High]
Does this regression leave the reserved CS value unfiltered? In
smmu_filter_command(), if the host submits a CMD_SYNC command with the CS
field set to 0b11 (Reserved), the filter only sanitizes MSIADDR if CS is
exactly 0b01 (IRQ). Thus, a command with CS=0b11 could bypass the filter
unmodified. If the hardware implementation aliases 0b11 to 0b01, this could
allow the host to perform arbitrary DMA writes to hypervisor memory.

> +	default:
> +		/* Deny unknown commands */
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> +{
> +	u64 *host_cmdq = hyp_phys_to_virt(smmu->cmdq_host.base_dma);
> +	bool use_wfe = smmu->features & ARM_SMMU_FEAT_SEV;
> +	u64 cmd[CMDQ_ENT_DWORDS];
> +	int idx, ret;
> +	u32 pending;
> +	bool skip;
> +
> +	if (!is_cmdq_enabled(smmu))
> +		return 0;
> +
> +	pending = (1 << (smmu->cmdq_host.llq.max_n_shift)) - queue_space(&smmu->cmdq_host.llq);
> +
> +	hyp_spin_lock(&smmu->hw_lock);
> +	/* Wait for the command queue to have some space. */
> +	ret = smmu_wait(use_wfe, smmu_cmdq_has_space(&smmu->cmdq, pending));
> +	if (ret) {
> +		hyp_spin_unlock(&smmu->hw_lock);
> +		return ret;
> +	}
> +
> +	while (pending--) {
> +		int i;
> +
> +		idx = Q_IDX(&smmu->cmdq_host.llq, smmu->cmdq_host.llq.cons);
> +		queue_inc_cons(&smmu->cmdq_host.llq);
> +
> +		/* Copy the command to local buffer avoiding TOCTOU */
> +		for (i = 0; i < CMDQ_ENT_DWORDS; ++i)
> +			cmd[i] = le64_to_cpu(READ_ONCE(host_cmdq[idx * CMDQ_ENT_DWORDS + i]));
> +
> +		skip = smmu_filter_command(smmu, cmd);
> +		if (WARN_ON(skip))
> +			continue;

[Severity: Medium]
Is this regression safe when using WARN_ON() for untrusted host inputs? At
EL2, WARN_ON() expands to a fatal BRK instruction, causing a hypervisor
panic. If the host writes a malformed command, this could allow a
host-triggered denial of service. Should the code error out or ignore the
command instead? I also noticed similar WARN_ON() assertions in
smmu_dabt_device() when handling an invalid register access size.

> +		smmu_add_cmd_raw(smmu, cmd);
> +	}
> +
> +	writel(smmu->cmdq.llq.prod, smmu->cmdq.prod_reg);
> +
> +	ret = smmu_wait(use_wfe, smmu_cmdq_empty(&smmu->cmdq));

[Severity: Low]
Could this regression cause the CPU to stall? The smmu_wait() loop uses wfe()
to wait for the hardware queue to empty, but the hardware only generates an
SEV on a CMD_SYNC command. If the host submits commands without a CMD_SYNC,
the CPU could stall in wfe() until a random interrupt wakes it up or the 1ms
timeout expires, leading to unnecessary latency.

> +	hyp_spin_unlock(&smmu->hw_lock);
> +	return ret;
> +}
[ ... ]
> @@ -381,18 +479,51 @@ static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
[ ... ]
>  	case ARM_SMMU_CMDQ_PROD:
> +		if (len != sizeof(u32))
> +			break;
> +		if (is_write) {
> +			smmu->cmdq_host.llq.prod = val;
> +			WARN_ON(smmu_emulate_cmdq_insert(smmu));

[Severity: Medium]
Could this regression allow multiple threads concurrently mutating
smmu->cmdq_host.llq.prod outside smmu->hw_lock to corrupt the queue state?
Modifying this value without a lock could allow concurrent host writes to
decouple the pending calculation from the consumer index.

Similarly, I see in smmu_dabt_device() that ARM_SMMU_CR0 is modified
locklessly when calling smmu_emulate_cmdq_enable(). Could concurrent writes
from multiple CPUs cause hypervisor panics due to double page
sharing/unsharing or queue desynchronization?

> +			goto out_ret;
> +		} else {
> +			val = smmu->cmdq_host.llq.prod;
> +			goto out_update_regs;
> +		}
>  	case ARM_SMMU_CMDQ_CONS:
> +		if (len != sizeof(u32))
> +			break;
> +		if (is_write) {
> +			if (WARN_ON(is_cmdq_enabled(smmu)))
> +				break;

[Severity: Medium]
Could this regression allow the host to panic the hypervisor by attempting an
invalid state transition (writing to CONS while the queue is enabled)? Like
the other WARN_ON() usages for host inputs, this should likely be an error
return instead of an assertion.

> +
> +			smmu->cmdq_host.llq.cons = val;
> +			goto out_ret;
> +		} else {
> +			/* Propagate errors back to the host.*/
> +			u32 cons = readl_relaxed(smmu->base + ARM_SMMU_CMDQ_CONS);
> +
> +			val = smmu->cmdq_host.llq.cons | (CMDQ_CONS_ERR & cons);
> +			goto out_update_regs;
> +		}
[ ... ]

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