Re: [PATCH v8 24/25] iommu/arm-smmu-v3-kvm: Enable nesting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu/arm-smmu-v3: missing ATTR_TYPES_OVR check before setting STE.SHCFG
--

commit 9a13859889ae6cece3b72c3b06628dae2d48b4cf
Author: Mostafa Saleh <smostafa@google.com>

iommu/arm-smmu-v3-kvm: Enable nesting

As now the hypervisor controls the command queue, stream table, and shadows
the stage-2 page table, we enable stage-2 translation in case the host puts
an STE in bypass or stage-1. The hypervisor will control the S2 part of the
STE and sanitize it.

> 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 5417a5c2bf58a..39c97404e0397 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
> @@ -461,6 +461,57 @@ static int smmu_init_cmdq(struct hyp_arm_smmu_v3_device *smmu)
>  	return 0;
>  }
>  
> +static int smmu_attach_stage_2(struct arm_smmu_ste *ste)
> +{
[ ... ]
> +	sl = pgt_cfg->arm_lpae_s2_cfg.vtcr.sl;
> +	ts = pgt_cfg->arm_lpae_s2_cfg.vtcr.tsz;
> +
> +	ste->data[1] &= ~cpu_to_le64(STRTAB_STE_1_SHCFG);
> +	ste->data[1] |= cpu_to_le64(FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING));

[Severity: High]
Does this code write to a RES0 field on hardware lacking the ATTR_TYPES_OVR
feature?

On hardware where the ATTR_TYPES_OVR feature is not supported, the
STRTAB_STE_1_SHCFG field is RES0. Writing a non-zero value to it triggers a
hardware event that aborts the transaction and breaks DMA functionally for
the device.

In the host kernel driver, this is guarded by a check for
smmu->features & ARM_SMMU_FEAT_ATTR_TYPES_OVR. Is it possible to add a
similar feature check here in smmu_attach_stage_2 before setting the
STRTAB_STE_1_SHCFG_INCOMING flag?

> +
> +	ste->data[1] &= ~cpu_to_le64(STRTAB_STE_1_EATS | STRTAB_STE_1_S2FWB | STRTAB_STE_1_STRW);
[ ... ]
> @@ -520,8 +599,7 @@ static int smmu_reshadow_ste(struct hyp_arm_smmu_v3_device *smmu, u32 sid, bool
>  		if (l1_idx >= cfg->l2.num_l1_ents)
>  			return -E2BIG;
>  
> -		host_ste_ptr = &host_ste_copy;
> -		ret = smmu_get_host_l2_ste(smmu, sid, host_ste_ptr);
> +		ret = smmu_get_host_l2_ste(smmu, sid, &target);
>  		if (ret)
>  			return ret;
>  
>  		/* No hypervisor entry, first time the L2 is populated. */
>  		if (!l1tab[l1_idx].l2ptr) {
>  			struct arm_smmu_strtab_l2 *l2table;
>  
>  			l2table = pkvm_iommu_alloc_pages(get_order(sizeof(*l2table)));
>  			if (!l2table)
>  				return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this out of memory condition trigger
a fatal panic at EL2?

When pkvm_iommu_alloc_pages fails here in smmu_reshadow_ste, the ENOMEM
error propagates up to smmu_filter_command.

The caller, smmu_emulate_cmdq_insert, will then hit WARN_ON(skip). At EL2,
WARN_ON expands to a fatal break instruction, crashing the entire host
system.

Since unprivileged host userspace can influence device mappings and exhaust
the hypervisor L2 table memory pool, can this error path be handled without
causing a hypervisor panic?

>  			/* Ensure L2 is observed before writing the ptr. */
>  			dma_wmb();
>  			arm_smmu_write_strtab_l1_desc(&l1tab[l1_idx], hyp_virt_to_phys(l2table));

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