Re: [PATCH RFCv2 01/13] genirq/msi: Store the IOMMU IOVA directly in msi_desc instead of iommu_cookie

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jan 23, 2025 at 06:10:48PM +0100, Eric Auger wrote:

> > However iommufd now permits the domain to change while the driver is
> > probed and VFIO userspace can create races with IRQ changes calling
> > iommu_dma_prepare/compose_msi_msg() and changing/freeing the iommu_domain.
> and is it safe in iommu_dma_prepare_msi()?

iommu_dma_prepare_msi() takes the group mutex:

int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
{
	struct device *dev = msi_desc_to_dev(desc);
	struct iommu_group *group = dev->iommu_group;

	mutex_lock(&group->mutex);
	if (group->domain && group->domain->sw_msi)
		ret = group->domain->sw_msi(group->domain, desc, msi_addr);

Which prevents changing domain attachments during execution.

For iommufd, if the domain attachment changes immediately after
iommu_dma_prepare_msi() unlocks, then the information given to
msi_desc_set_iommu_msi_iova() is still valid on the new domain.

This is because the iommufd implementation of sw_msi keeps the same
IOVA for the same ITS page globally across all domains. Any racing
change of domain will attach a new domain with the right ITS IOVA
already mapped and populated.

It is why this series stops using the domain pointer as a cookie
inside the msi_desc, immediately after the group->mutex is unlocked
a new domain can be attached and the old domain can be freed, which
would UAF the domain pointer in the cookie.

> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index b10093c4d00e..d442b4a69d56 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -184,7 +184,8 @@ struct msi_desc {
> >  	struct msi_msg			msg;
> >  	struct irq_affinity_desc	*affinity;
> >  #ifdef CONFIG_IRQ_MSI_IOMMU
> > -	const void			*iommu_cookie;
> you may add kernel doc comments above

I wondered if internal stuff was not being documented as the old
iommu_cookie didn't have a comment..

But sure:

 * @iommu_msi_iova: Optional IOVA from the IOMMU to overide the msi_addr.
 *                  Only used if iommu_msi_page_shift != 0
 * @iommu_msi_page_shift: Indicates how many bits of the original address
 *                        should be preserved when using iommu_msi_iova.

Jason