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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:49:58PM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

Technically true, although this was unchecked for the kernel for years,
so I doubt any HW cares, but I will fix it.

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


Same as discussed before.

Thanks,
Mostafa

> 
> >  			/* 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