Re: [PATCH v8 13/25] iommu/arm-smmu-v3-kvm: Add MMIO emulation

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:22:32PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] hyp: fatal WARN_ON(!mask) on host SMMU MMIO access
> --
> 
> commit 0f61ec17741805782c471d3c6da0d9dad62368e6
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Add MMIO emulation
> 
> Add data abort handler for the SMMUs, at the moment most registers
> are just passthrough.
> 
> > 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 f8b7c5406017e..7a51cb70205fe 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
> [ ... ]
> > +static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
> > +			     struct user_pt_regs *regs,
> > +			     u64 esr, u32 off)
> > +{
> [ ... ]
> > +	case ARM_SMMU_CR1:
> > +		if (len != sizeof(u32))
> > +			break;
> > +		/* Don't mess with shareability/cacheability. */
> > +		if (is_write &&
> > +		    (val != (FIELD_PREP(CR1_TABLE_SH, ARM_SMMU_SH_ISH) |
> > +			     FIELD_PREP(CR1_TABLE_OC, CR1_CACHE_WB) |
> > +			     FIELD_PREP(CR1_TABLE_IC, CR1_CACHE_WB) |
> > +			     FIELD_PREP(CR1_QUEUE_SH, ARM_SMMU_SH_ISH) |
> > +			     FIELD_PREP(CR1_QUEUE_OC, CR1_CACHE_WB) |
> > +			     FIELD_PREP(CR1_QUEUE_IC, CR1_CACHE_WB))))
> > +			break;
> > +
> > +		mask = read_write;
> > +		break;
> [ ... ]
> > +	}
> > +
> > +	if (WARN_ON(!mask))
> > +		goto out_ret;
> 
> [Severity: Medium]
> Is it safe to use WARN_ON() here to validate host-controlled input?
> 
> If the host writes an unallowed value to ARM_SMMU_CR1 (breaking out of the
> switch before setting mask), or accesses an unhandled register offset,
> mask remains 0. The subsequent WARN_ON(!mask) will trigger a fatal
> BRK instruction at EL2, crashing the hypervisor and the host.
> 
> Since WARN_ON() is fatal at EL2 and does not execute the goto out_ret
> recovery path, should this validation gracefully handle the abort instead of
> asserting?

Yes, no well behaved host will attempt this, so a WARN_ON() is better
than ignoring it and making it really hard to debug.

Thanks,
Mostafa

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