Re: [PATCH v9 17/25] iommu/arm-smmu-v3-kvm: Shadow stream table

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:30:54PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] iommu/arm-smmu: missing length check for STRTAB_BASE corrupts hyp state
> - [Low] iommu/arm-smmu-v3-kvm: Incorrect WARN_ON usage panics hypervisor on invalid host config
> --
> 
> commit 9c237aa634fe8fc969bb3e8a3f07ec53bd303b77
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Shadow stream table
> 
> This commit allocates the shadow stream table per SMMU, sized up to 1MB
> for the 2-level host table case. Host writes remain passthrough for now,
> and the stream table is shared/unshared on SMMU enable/disable.
> 
> > 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
> [ ... ]
> > @@ -441,6 +502,46 @@ static int smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> [ ... ]
> > +static void smmu_emulate_enable(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	/* Enabling SMMU without CMDQ, means TLB invalidation won't work. */
> > +	if (WARN_ON(!is_cmdq_enabled(smmu)))
> > +		return;
> > +
> > +	WARN_ON(smmu_update_ste_shadow(smmu, true));
> > +}
> 
> [Severity: Low]
> Does this panic the hypervisor on invalid host configuration?  These
> assertions evaluate host-supplied states and configurations, which causes a
> fatal EL2 BUG() when the host makes an invalid transition.
> 
> According to the pKVM Threat Model, host availability against itself is not
> guaranteed, making this a hardening improvement rather than a security bug.
> However, per the pKVM WARN_ON semantics guide, WARN_ON must only be used
> for EL2 internal invariant violations.  Validating host-supplied inputs with
> WARN_ON is a correctness violation, as the hypervisor should gracefully
> handle or ignore invalid state transitions rather than panicking.

Same as replied in v8:
https://lore.kernel.org/all/arKjzVDb56JmHJMc@google.com/

> 
> [ ... ]
> > @@ -528,7 +629,23 @@ static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
> >  		}
> >  	/* Passthrough the register access for bisectability, handled later */
> >  	case ARM_SMMU_STRTAB_BASE:
> > +		if (is_write) {
> > +			/* Must only be written when SMMU_CR0.SMMUEN == 0. */
> > +			if (is_smmu_enabled(smmu))
> > +				break;
> > +			smmu->host_ste_base = val;
> > +		}
> > +		mask = read_write;
> > +		break;
> 
> [Severity: High]
> Is a length check missing here?  The missing len == sizeof(u64) check
> allows a 32-bit access to capture the full 64-bit CPU register, corrupting
> host_ste_base.
> 
> A 32-bit write to STRTAB_BASE will assign the entire 64-bit
> regs->regs[rd] (including uninitialized upper 32 bits) to
> smmu->host_ste_base.  Unlike the ARM_SMMU_CMDQ_BASE trap handler which
> explicitly ensures the access is the correct width (if (len !=
> sizeof(u64)) break;), this handler omits the validation.
> 

Yes, for bisectability, that is fixed next.

> >  	case ARM_SMMU_STRTAB_BASE_CFG:
> > +		if (is_write) {
> > +			/* Must only be written when SMMU_CR0.SMMUEN == 0. */
> > +			if (is_smmu_enabled(smmu))
> > +				break;
> > +			smmu->host_ste_cfg = val;
> > +		}
> > +		mask = read_write;
> > +		break;
> 
> [Severity: High]
> Similarly, is a length check missing for this 32-bit register?  The missing
> len == sizeof(u32) check allows 64-bit writes to pass through to a 32-bit
> register offset.
> 
> A 64-bit write to STRTAB_BASE_CFG will invoke writeq_relaxed() on a
> 32-bit register offset (0x88), causing an unaligned or oversized write to
> Device memory which triggers a fatal EL2 alignment fault.

Same as above.

Thanks,
Mostafa

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