Re: [PATCH v8 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:
- [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?

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

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

> +			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?

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