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

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:28:01PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] iommu/arm-smmu-v3-kvm: wrong WARN_ON() causes hyp panic on invalid host MMIO
> - [Medium] iommu/arm-smmu-v3-kvm: WARN_ON used to validate host input in SMMU emulation
> --
> 
> commit bf6dcb76ab39be1d82600f47f81d772f4c08b9a6
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Shadow the command queue
> 
> This patch allocates a shadow command queue per SMMU to be used by the
> hypervisor at boot. The host command queue is pinned in a shared state
> when enabled to avoid donation issues and prevent untrusted host access
> from reaching the hypervisor state.
> 
> > 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
> > --- 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
> 
> [ ... ]
> 
> > @@ -132,6 +225,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]
> Will this cause a fatal hypervisor panic if the host provides an invalid
> physical address or an unshareable memory range?
> 
> At EL2, the WARN_ON() macro behaves as a fatal exception. If the host
> configures the emulated SMMU with an invalid address, smmu_share_pages()
> can fail and return an error. Wrapping this fallible, host-influenced
> operation in WARN_ON() will crash the hypervisor. Could this path safely
> ignore the invalid configuration or return an error instead?
> 

By design as replied in v8:
https://lore.kernel.org/all/20260922133309.CD1F51F000FF@smtp.kernel.org/

> > +}
> > +
> > +static void smmu_emulate_cmdq_disable(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	WARN_ON(smmu_unshare_pages(smmu->cmdq_host.base_dma,
> > +				   cmdq_size(&smmu->cmdq_host)));
> 
> [Severity: Medium]
> Does this also have the same issue with WARN_ON() panicking the hypervisor
> if the host-provided memory range fails to unshare?

By design as above.

> 
> > +}
> > +
> 
> [ ... ]
> 
> > @@ -158,6 +268,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;
> 
> [Severity: Medium]
> If the host kernel makes an invalid MMIO write to ARM_SMMU_CMDQ_BASE while
> the CMDQ is enabled, the switch statement breaks and leaves the mask set
> to 0.
> 
> Later in smmu_dabt_device(), there is a pre-existing check:
> 
> 	if (WARN_ON(!mask))
> 		goto out_ret;
> 
> Since WARN_ON() is fatal at EL2, will this turn an invalid host MMIO access
> into a hypervisor panic? It seems like the access should be gracefully
> ignored or rejected without triggering a fatal assertion.

Yes, by design, if the host makes violates the architecture we just
panic.

Thanks,
Mostafa

> 
> > +			smmu->cmdq_host.q_base = val;
> > +		}
> > +		mask = read_write;
> > +		break;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=14