Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 18 Apr 2024 09:02:24 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH] iommu: Fix def_domain_type interaction with untrusted devices |
| |
On Thu, Apr 18, 2024 at 05:14:59PM +0530, Vasant Hegde wrote: > We return IDENTITY only if SNP and memory encryption is not enabled and device > is SVA capable. Upstream has below code (v6.9-rc4) > > if (pdev_pasid_supported(dev_data) && > !cc_platform_has(CC_ATTR_MEM_ENCRYPT) && > !amd_iommu_snp_en) { > return IOMMU_DOMAIN_IDENTITY; > } > > > and during boot, we will enforce Paging domain when Encryption/SNP is enabled.
It is very confusing considering the comment says identity isn't supported:
* Do not identity map IOMMUv2 capable devices when: * - memory encryption is active, because some of those devices * (AMD GPUs) don't have the encryption bit in their DMA-mask * and require remapping. * - SNP is enabled, because it prohibits DTE[Mode]=0. */
Then it goes on to return IDENTITY anyhow.
If the HW cannot identity map it needs to return DMA for those cases:
/* SNP is enabled, because it prohibits DTE[Mode]=0, IDENTITY is not * supported */ if (amd_snp_en) return IOMMU_DOMAIN_DMA;
/* memory encryption is active, because some of those devices * (AMD GPUs) don't have the encryption bit in their DMA-mask * and require remapping. IDENTITY is not supported. */ if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) return IOMMU_DOMAIN_DMA;
> So far the policy was core layer to handle untrusted device which got changed > and hence we hit this bug.
Well I would say the AMD driver changed to mis-use def_domain for policy and that causes problems.
> I don't see any issue with enforcing these checks inside AMD driver.. If we go > with this approach then IMO core should just adhere to what driver returned > rather than failing. Having policy decision at two level is inviting trouble > like this one.
Again, no policy in drivers.
def_domain should return only the HW capability. If it returns IDENTITY then the driver must fail attaches of PAGING.
You should send a patch fixing this function and remove the PASID test entirely for now.
Jason
| ![\](/images/icornerr.gif) |