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

[sashiko-bot@kernel.org]

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.

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

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

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