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

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:30:32PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Critical] iommu/arm-smmu-v3-kvm: user-triggerable hyp panic via unhandled SMMU commands (e.g., CMDQ_OP_ATC_INV)
> - [Critical] iommu/arm-smmu-v3-kvm: CMD_SYNC filter allows arbitrary memory writes via SIG32
> - [High] iommu: EL2 panic on SMMU hardware timeout or CERROR due to WARN_ON
> - [Medium] arm-smmu-v3-kvm: Dead-branch traps due to WARN_ON() panicking at EL2
> --
> 
> commit 44f2b5e9d53208cadd7d659b59bf05189d20f72d
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Emulate CMDQ for host
> 
> This commit introduces command queue emulation for the host in the ARM SMMUv3
> pKVM driver. It filters command queue entries, shadows the queue, and prevents
> the host from directly manipulating the hardware command queue to ensure isolation.
> 
> > 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
> > index c9fc21e405e79..dc9b1e659a422 100644
> > --- 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
> [ ... ]
> > +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_TLBI_NH_ASID:
> > +	case CMDQ_OP_TLBI_NH_VA:
> > +	case CMDQ_OP_TLBI_NH_ALL:
> > +	case 0x13: /* CMD_TLBI_NH_VAA: Not used by Linux */
> > +	{
> > +		/* Only allow VMID = 0 */
> > +		if (FIELD_GET(CMDQ_TLBI_0_VMID, command[0]) != 0)
> > +			return true;
> 
> [Severity: Critical]
> Could this reject valid nested-translation TLBI commands? A host userspace
> process with IOMMUFD nested translation might legitimately trigger invalidation
> commands (like CMDQ_OP_TLBI_NH_VA) where the host injects a non-zero VMID.
> 
> Because this returns true, the command gets rejected, which later leads to a
> hypervisor panic (see below) triggered directly from userspace.
> 

Wrong, the host kernel only sees a single stage SMMUv3, so it can't issue
non zero VMID or IPA invalidations.

> > +		break;
> > +	}
> > +	case CMDQ_OP_PREFETCH_CFG:
> > +	case CMDQ_OP_CFGI_CD:
> > +	case CMDQ_OP_CFGI_CD_ALL:
> > +	case CMDQ_OP_TLBI_NSNH_ALL:
> > +	case CMDQ_OP_PRI_RESP:
> > +	case CMDQ_OP_RESUME:
> > +		break;
> > +	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;
> > +		}
> 
> [Severity: Critical]
> Does this allow the host to inject an SMMU hardware instruction that executes
> an arbitrary physical memory write? The filter specifically neutralizes CS=1
> (IRQ), but it appears to ignore CS=3 (SIG32). If a malicious host issues a
> CMD_SYNC with CS=3, the hardware might perform a 32-bit physical memory write
> of the host-provided MSIData to the host-provided MSIAddress, compromising the
> hypervisor or guest memory.

As mentioned in v8:
https://lore.kernel.org/all/20260922133237.5A5381F000FF@smtp.kernel.org/

This is not true, there is no such encoding in the spec.

> 
> > +		break;
> > +	default:
> > +		/* Deny unknown commands */
> > +		return true;
> 
> [Severity: Critical]
> Could valid user-triggerable commands hit this default case? For example,
> CMDQ_OP_ATC_INV appears to be missing from the allowlist. Since it returns
> true, this will also result in a user-triggerable hypervisor panic when the
> caller evaluates the rejection.
> 

ATS is hidden from IDR0, the kernel shouldn't attempt to use it.

> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static int smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> [ ... ]
> > +		skip = smmu_filter_command(smmu, cmd);
> > +		if (WARN_ON(skip))
> > +			continue;
> 
> [Severity: Medium]
> Will this execute the continue statement? At EL2 in nVHE/pKVM, WARN_ON() expands
> to a BRK instruction and triggers a fatal hypervisor panic. Because the macro
> doesn't return, the error-handling path is dead code, and rejecting a command
> results in a hard panic.
> 
> This also elevates the severity of the filter rejections mentioned above. If
> userspace can trigger a rejected command (like CMDQ_OP_ATC_INV or a nested
> TLBI with VMID != 0), it becomes a Denial of Service attack against the host
> system.
> 

Again, by design, any unsupported command will panic, that can be
relaxed, but that only happens if the kernel is malicous or buggy so
I'd prefer to catch the place caused the issue to make that easier to
debug.

> > +		smmu_add_cmd_raw(smmu, cmd);
> > +	}
> > +
> > +	writel(smmu->cmdq.llq.prod, smmu->cmdq.prod_reg);
> > +
> > +	ret = smmu_wait(false, smmu_cmdq_empty(&smmu->cmdq));
> > +	hyp_spin_unlock(&smmu->hw_lock);
> > +	return ret;
> > +}
> [ ... ]
> > +static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
> > +			     struct user_pt_regs *regs,
> > +			     u64 esr, u32 off)
> > +{
> [ ... ]
> > +	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: High]
> Can a hardware timeout or CERROR cause a hypervisor panic here?
> smmu_emulate_cmdq_insert() relies on smmu_wait() to check if the hardware
> command queue is empty. If the SMMU hardware halts due to an error or takes
> too long under heavy load, smmu_wait() returns -ETIMEDOUT.
> 
> Because the return value is wrapped in WARN_ON(), this timeout will be
> converted into a fatal EL2 panic instead of propagating the error.
> 

Yes, if the HW hangs, we can't return as the TLB state might leak
donated memory, so in that case we have to panic.

> > +			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]
> Similar to the dead-branch trap above, the break statement here appears to be
> unreachable code. Because WARN_ON() expands to a fatal BUG() at EL2, writing
> to the CMDQ_CONS register when the queue is enabled will panic the hypervisor
> instead of just ignoring the write.

Yes, that just looks cleaner, similar patterns used in mem_protect.c,
compilers are smart now and will remove it.

Thanks,
Mostafa

> 
> > +
> > +			smmu->cmdq_host.llq.cons = val;
> > +			goto out_ret;
> > +		} else {
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=16