Commit d6c55c0a authored by Jason Gunthorpe's avatar Jason Gunthorpe
Browse files

iommufd: Change the order of MSI setup 

Eric points out this is wrong for the rare case of someone using
allow_unsafe_interrupts on ARM. We always have to setup the MSI window in
the domain if the iommu driver asks for it.

Move the iommu_get_msi_cookie() setup to the top of the function and
always do it, regardless of the security mode. Add checks to
iommufd_device_setup_msi() to ensure the driver is not doing something
incomprehensible. No current driver will set both a HW and SW MSI window,
or have more than one SW MSI window.

Fixes: e8d57210 ("iommufd: Add kAPI toward external drivers for physical devices")
Link: https://lore.kernel.org/r/3-v1-0362a1a1c034+98-iommufd_fixes1_jgg@nvidia.com


Reviewed-by: default avatarKevin Tian <kevin.tian@intel.com>
Reported-by: default avatarEric Auger <eric.auger@redhat.com>
Signed-off-by: default avatarJason Gunthorpe <jgg@nvidia.com>
parent a26fa392

drivers/iommu/iommufd/device.c

+25 −31
Original line number Diff line number Diff line
@@ -139,19 +139,11 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev, 
	int rc;



	/*

	 * IOMMU_CAP_INTR_REMAP means that the platform is isolating MSI, and it

	 * creates the MSI window by default in the iommu domain. Nothing

	 * further to do.

	 */

	if (device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP))

		return 0;



	/*

	 * On ARM systems that set the global IRQ_DOMAIN_FLAG_MSI_REMAP every

	 * allocated iommu_domain will block interrupts by default and this

	 * special flow is needed to turn them back on. iommu_dma_prepare_msi()

	 * will install pages into our domain after request_irq() to make this

	 * work.

	 * If the IOMMU driver gives a IOMMU_RESV_SW_MSI then it is asking us to

	 * call iommu_get_msi_cookie() on its behalf. This is necessary to setup

	 * the MSI window so iommu_dma_prepare_msi() can install pages into our

	 * domain after request_irq(). If it is not done interrupts will not

	 * work on this domain.

	 *

	 * FIXME: This is conceptually broken for iommufd since we want to allow

	 * userspace to change the domains, eg switch from an identity IOAS to a
@@ -159,33 +151,35 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev, 
	 * matches what the IRQ layer actually expects in a newly created

	 * domain.

	 */

	if (irq_domain_check_msi_remap()) {

		if (WARN_ON(!sw_msi_start))

			return -EPERM;

	if (sw_msi_start != PHYS_ADDR_MAX && !hwpt->msi_cookie) {

		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);

		if (rc)

			return rc;



		/*

		 * iommu_get_msi_cookie() can only be called once per domain,

		 * it returns -EBUSY on later calls.

		 */

		if (hwpt->msi_cookie)

			return 0;

		rc = iommu_get_msi_cookie(hwpt->domain, sw_msi_start);

		if (rc)

			return rc;

		hwpt->msi_cookie = true;

		return 0;

	}



	/*

	 * Otherwise the platform has a MSI window that is not isolated. For

	 * historical compat with VFIO allow a module parameter to ignore the

	 * insecurity.

	 * For historical compat with VFIO the insecure interrupt path is

	 * allowed if the module parameter is set. Insecure means that a MemWr

	 * operation from the device (eg a simple DMA) cannot trigger an

	 * interrupt outside this iommufd context.

	 */

	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP) &&

	    !irq_domain_check_msi_remap()) {

		if (!allow_unsafe_interrupts)

			return -EPERM;



		dev_warn(

			idev->dev,

		"MSI interrupt window cannot be isolated by the IOMMU, this platform is insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");

			"MSI interrupts are not secure, they cannot be isolated by the platform. "

			"Check that platform features like interrupt remapping are enabled. "

			"Use the \"allow_unsafe_interrupts\" module parameter to override\n");

	}

	return 0;

}
@@ -203,7 +197,7 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, 
static int iommufd_device_do_attach(struct iommufd_device *idev,

				    struct iommufd_hw_pagetable *hwpt)

{

	phys_addr_t sw_msi_start = 0;

	phys_addr_t sw_msi_start = PHYS_ADDR_MAX;

	int rc;



	mutex_lock(&hwpt->devices_lock);

drivers/iommu/iommufd/io_pagetable.c

+14 −10
Original line number Diff line number Diff line
@@ -1170,6 +1170,8 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt, 
	struct iommu_resv_region *resv;

	struct iommu_resv_region *tmp;

	LIST_HEAD(group_resv_regions);

	unsigned int num_hw_msi = 0;

	unsigned int num_sw_msi = 0;

	int rc;



	down_write(&iopt->iova_rwsem);
@@ -1181,23 +1183,25 @@ int iopt_table_enforce_group_resv_regions(struct io_pagetable *iopt, 
		if (resv->type == IOMMU_RESV_DIRECT_RELAXABLE)

			continue;



		/*

		 * The presence of any 'real' MSI regions should take precedence

		 * over the software-managed one if the IOMMU driver happens to

		 * advertise both types.

		 */

		if (sw_msi_start && resv->type == IOMMU_RESV_MSI) {

			*sw_msi_start = 0;

			sw_msi_start = NULL;

		}

		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI)

		if (sw_msi_start && resv->type == IOMMU_RESV_MSI)

			num_hw_msi++;

		if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI) {

			*sw_msi_start = resv->start;

			num_sw_msi++;

		}



		rc = iopt_reserve_iova(iopt, resv->start,

				       resv->length - 1 + resv->start, device);

		if (rc)

			goto out_reserved;

	}



	/* Drivers must offer sane combinations of regions */

	if (WARN_ON(num_sw_msi && num_hw_msi) || WARN_ON(num_sw_msi > 1)) {

		rc = -EINVAL;

		goto out_reserved;

	}



	rc = 0;

	goto out_free_resv;