Re: [PATCH v8 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] hyp: wrong WARN_ON() in smmu_emulate_cmdq_enable() causes panic
- [Medium] hyp: WARN_ON() on host MMIO access creates dead-branch trap
--

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

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

At boot, allocate a command queue per SMMU which is used as a shadow
by the hypervisor. The host command queue is pinned in a shared state
to prevent it from being donated to VMs.

> 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 7a51cb70205fe..b26c21b595b47 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
[ ... ]
> @@ -132,6 +222,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]
If the host provides an invalid or unmapped command queue base address via
the ARM_SMMU_CMDQ_BASE register, smmu_share_pages() will return an error.
Since WARN_ON() expands to a fatal break instruction at EL2, would this
allow the host to intentionally crash the hypervisor by passing bad input?

Could we handle the smmu_share_pages() failure gracefully instead of using
WARN_ON() on host-controlled inputs?

> +}
> +
[ ... ]
> @@ -158,6 +265,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;
> +			smmu->cmdq_host.q_base = val;
> +		}
> +		mask = read_write;
> +		break;

[Severity: Medium]
When the host writes to ARM_SMMU_CMDQ_BASE while the command queue is
enabled, the switch statement in smmu_dabt_device() breaks with mask
remaining zero.

Later in smmu_dabt_device(), there is an existing check:

> 	if (WARN_ON(!mask))
> 		goto out_ret;

Because WARN_ON() at EL2 causes a fatal panic, the goto out_ret recovery
path is dead code. Would this allow the host to crash the hypervisor by
performing an invalid write to ARM_SMMU_CMDQ_BASE?

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