Re: [PATCH] iommu: Fix def_domain_type interaction with untrusted devices

[Jason Gunthorpe]

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