Re: [PATCH v8 14/25] iommu/arm-smmu-v3-kvm: Shadow the command queue

[Mostafa Saleh <smostafa@google.com>]

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

No it couldn't, this only happen if the host is malicious or gone too
wrong.

> 
> > +}
> > +
> [ ... ]
> > @@ -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.

Yes, because that is not allowed by the architecture and no well
behaved host will attempt that.

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

Yes, the host can crash the hypervisor in many ways.

Thanks,
Mostafa




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