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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:32:36PM +0000, sashiko-bot@kernel.org wrote:
> 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.

No, according to the spec:
    0b11: Reserved: Causes a CERROR_ILL.

> 
> > +	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.

Yes, no well behaved host will issue unsupported commands, so WARN_ON
should be OK here.

> 
> > +		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.

True, I think we should pass WFE as an arg to this macro which is used for
certain conditions that are known to send an event. I will fix that.

> 
> > +	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?
> 

Wrong, smmu->cmdq_host.llq.prod is protected by the smmu->lock and the
not hw_lock which only protects the HW command queue.
And there is no part of the code where this is read or written without
the smmu->lock.

Also wrong for ARM_SMMU_CR0, the caller takes the lock.

> > +			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.
> 

Yes as mentioned before this is not allowed by the architecture, so
no well-behaved host will do that and WARN_ON should be OK.

Thanks,
Mostafa

> > +
> > +			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