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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu: mmio emulation bitwise logic corrupted on big-endian hosts
--

commit 5a437530a55c608782e029a478141ad9b342c024
Author: Mostafa Saleh <smostafa@google.com>

iommu/arm-smmu-v3-kvm: Add MMIO emulation

This commit adds a data abort handler for the SMMUs to handle MMIO emulation.
Currently, most registers act as a passthrough, and upcoming patches will
add CMDQ/STE emulation with logic for specific register access.

> 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
[ ... ]
> @@ -128,6 +132,170 @@ static int smmu_init(void)
> +static bool smmu_dabt_device(struct hyp_arm_smmu_v3_device *smmu,
> +			     struct user_pt_regs *regs,
> +			     u64 esr, u32 off)
> +{
[ ... ]
> +	bool is_xzr = (rd == 31);
> +	u64 val = is_xzr ? 0 : regs->regs[rd];

[Severity: High]
Does extracting the register value directly from the CPU context here bypass
endianness conversion on big-endian hosts?

When the host (EL1) uses writel_relaxed() on a big-endian machine, it
byte-swaps the native value to little-endian representation before issuing
the store. The MMIO trap then retrieves this raw, byte-swapped value
directly into val.

> +
> +	switch (off) {
> +	case ARM_SMMU_IDR0:
[ ... ]
> +	case ARM_SMMU_CR0:
> +		if (len != sizeof(u32))
> +			break;
> +
> +		if (is_write) {
> +			/* Enforce ATS checks. */
> +			if (smmu->features & ARM_SMMU_FEAT_ATS)
> +				val |= CR0_ATSCHK;

[Severity: High]
Can this bitwise operation corrupt the configuration on big-endian hosts?

Because val contains a byte-swapped value, applying a native bitmask like
CR0_ATSCHK modifies bits at the wrong offset.

> +			mask = read_write;
> +		} else {
> +			mask = read_write & ~CR0_ATSCHK;
> +		}
> +		break;
[ ... ]
> +	if (is_write) {
> +		if (len == sizeof(u64))
> +			writeq_relaxed(val & mask, smmu->base + off);
> +		else
> +			writel_relaxed(val & mask, smmu->base + off);

[Severity: High]
Will this double-swap the corrupted value?

Since val was already byte-swapped by the host's write, writing it back
using EL2's writel_relaxed() applies a second byte-swap, propagating the
incorrect state to the SMMU hardware.

Read paths face a similar issue where the readl_relaxed() output is passed
directly to the guest without being swapped back into the format expected by
the guest's own le32_to_cpu() calls.

> +
> +		hyp_spin_unlock(&smmu->hw_lock);
> +		return true;
> +	}

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