Re: [PATCH v9 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:
- [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.

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

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

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

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

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

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