linux-kernel - Re: [PATCH v2 1/4] iommu: Add private_data_owner to iommu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2754908e-f345-43ca-9cb1-9300abe5d402@arm.com>
Date: Fri, 28 Feb 2025 16:29:17 +0000
From: Robin Murphy <robin.murphy@....com>
To: Nicolin Chen <nicolinc@...dia.com>, jgg@...dia.com, kevin.tian@...el.com,
 joro@...tes.org, will@...nel.org
Cc: iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] iommu: Add private_data_owner to iommu_domain

On 28/02/2025 1:31 am, Nicolin Chen wrote:
> Steal two bits from the 32-bit "type" in struct iommu_domain to store a
> new tag for private data owned by either dma-iommu or iommufd.
> 
> Set the domain->private_data_owner in dma-iommu and iommufd. These will
> be used to replace the sw_msi function pointer.
> 
> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
>   include/linux/iommu.h                | 7 ++++++-
>   drivers/iommu/dma-iommu.c            | 2 ++
>   drivers/iommu/iommufd/hw_pagetable.c | 3 +++
>   3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e93d2e918599..4f2774c08262 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -209,8 +209,13 @@ struct iommu_domain_geometry {
>   #define IOMMU_DOMAIN_PLATFORM	(__IOMMU_DOMAIN_PLATFORM)
>   #define IOMMU_DOMAIN_NESTED	(__IOMMU_DOMAIN_NESTED)
>   
> +#define IOMMU_DOMAIN_DATA_OWNER_NONE (0U)
> +#define IOMMU_DOMAIN_DATA_OWNER_DMA (1U)
> +#define IOMMU_DOMAIN_DATA_OWNER_IOMMUFD (2U)
> +
>   struct iommu_domain {
> -	unsigned type;
> +	u32 type : 30;
> +	u32 private_data_owner : 2;
>   	const struct iommu_domain_ops *ops;
>   	const struct iommu_dirty_ops *dirty_ops;
>   	const struct iommu_ops *owner; /* Whose domain_alloc we came from */
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 94263ed2c564..78915d74e8fa 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -403,6 +403,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>   
>   	mutex_init(&domain->iova_cookie->mutex);
>   	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
> +	domain->private_data_owner = IOMMU_DOMAIN_DATA_OWNER_DMA;
>   	return 0;
>   }
>   
> @@ -435,6 +436,7 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
>   	cookie->msi_iova = base;
>   	domain->iova_cookie = cookie;
>   	iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);
> +	domain->private_data_owner = IOMMU_DOMAIN_DATA_OWNER_DMA;

This doesn't help all that much when it can still be called on any old 
unmanaged domain and silently overwrite whatever the previous user 
thought they owned...

And really the "owner" of MSI cookies is VFIO, it just happens that all 
the code for them still lives in iommu-dma because it was written to 
piggyback off the same irqchip hooks. TBH I've long been keen on 
separating them further from "real" IOVA cookies, and generalising said 
hooks really removes any remaining reason to keep the implementations 
tied together at all, should they have cause to diverge further (e.g. 
with makign the MSI cookie window programmable). What I absolutely want 
to avoid is a notion of having two types of MSI-mapping cookies, one of 
which is two types of MSI-mapping cookies.

Hopefully that is all clear in the patch I proposed.

Thanks,
Robin.

>   	return 0;
>   }
>   EXPORT_SYMBOL(iommu_get_msi_cookie);
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 7de6e914232e..5640444de475 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -157,6 +157,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>   		}
>   	}
>   	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
> +	hwpt->domain->private_data_owner = IOMMU_DOMAIN_DATA_OWNER_IOMMUFD;
>   
>   	/*
>   	 * Set the coherency mode before we do iopt_table_add_domain() as some
> @@ -253,6 +254,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
>   	}
>   	hwpt->domain->owner = ops;
>   	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
> +	hwpt->domain->private_data_owner = IOMMU_DOMAIN_DATA_OWNER_IOMMUFD;
>   
>   	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
>   		rc = -EINVAL;
> @@ -310,6 +312,7 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
>   	}
>   	hwpt->domain->owner = viommu->iommu_dev->ops;
>   	iommu_domain_set_sw_msi(hwpt->domain, iommufd_sw_msi);
> +	hwpt->domain->private_data_owner = IOMMU_DOMAIN_DATA_OWNER_IOMMUFD;
>   
>   	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
>   		rc = -EINVAL;