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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:33:09PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] arm-smmu-v3-kvm: unhandled break in switch statement causes EL2 panic on STRTAB access
> - [Medium] arm-smmu-v3-kvm: improper WARN_ON() usage for host inputs allows trivial EL2 panics
> --
> 
> commit a05ba1b1951c6df5b35f980e9826eb9d8a4ea188
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Shadow stream table
> 
> This patch allocates the shadow stream table per SMMU with a maximum size
> of 1MB for 2-level tables. It sets up passthrough for host writes to the
> stream table base and configuration registers, laying groundwork for future
> CFGI command trapping.
> 
> > 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
> > --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> [ ... ]
> > @@ -438,6 +499,46 @@ static int smmu_emulate_cmdq_insert(struct hyp_arm_smmu_v3_device *smmu)
> >  	return ret;
> >  }
> >  
> > +static int smmu_update_ste_shadow(struct hyp_arm_smmu_v3_device *smmu, bool enabled)
> [ ... ]
> > +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;
> 
> [Severity: Medium]
> Could this WARN_ON panic EL2 if the host enables the SMMU without enabling
> the CMDQ? Since WARN_ON at EL2 behaves as a fatal panic, using it for
> host-controlled inputs converts an invalid host state into a hypervisor DoS.
> Would returning an error be safer here?
> 

Yes, as I replied before.

> > +
> > +	WARN_ON(smmu_update_ste_shadow(smmu, true));
> > +}
> 
> [Severity: Medium]
> Similarly, does this panic EL2 if the host provides an invalid stream table
> configuration? The pKVM guidelines suggest that EL2 should not assert on
> contracted host inputs.
> 
> [ ... ]
> > @@ -525,7 +626,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;
> 
> [Severity: Medium]
> Does this break statement erroneously exit the entire switch block for
> ARM_SMMU_STRTAB_BASE? If the host writes to this register while the SMMU is
> enabled, the break bypasses the mask assignment, leaving mask = 0. Later in
> the function, if (WARN_ON(!mask)) evaluates to true, which triggers a fatal
> hypervisor panic.

Not an error, that is the intent, a panic will happen if the host
violate the spec.

> 
> > +			smmu->host_ste_base = val;
> > +		}
> > +		mask = read_write;
> > +		break;
> >  	case ARM_SMMU_STRTAB_BASE_CFG:
> > +		if (is_write) {
> > +			/* Must only be written when SMMU_CR0.SMMUEN == 0.*/
> > +			if (is_smmu_enabled(smmu))
> > +				break;
> 
> [Severity: Medium]
> Could this break statement similarly cause an EL2 panic on STRTAB_BASE_CFG
> access? Like the case above, this appears to leave mask = 0 and panic the
> hypervisor.

Same as above.

> 
> > +			smmu->host_ste_cfg = val;
> > +		}
> > +		mask = read_write;
> > +		break;
> >  	case ARM_SMMU_GBPA:
> [ ... ]
> 
> [Severity: Medium]
> Are there other instances where WARN_ON is used to validate host inputs?
> For example, in smmu_emulate_cmdq_insert():
> 
> smmu_emulate_cmdq_insert()
>     skip = smmu_filter_command(smmu, cmd);
>     if (WARN_ON(skip))
>         continue;
> 
> And at the end of smmu_dabt_device():
> 
> smmu_dabt_device()
>     if (WARN_ON(!mask))
>         goto out_ret;
> 
> Since these are directly reachable by the host kernel, they allow a trivial
> host self-DoS by inserting an unsupported command or performing an unhandled
> access length. Could these assertions be replaced with standard error
> handling or abort injection?
> 

Same as above.

Thanks,
Mostafa

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