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: e8d57210035b ("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: Kevin Tian <kevin.tian@intel.com>
Reported-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index dd2a415b603e3b2fa6fa90832b2826cb066298c2..d81f93a321afcb7a79e5e69cb755ff008ab96f6b 100644 (file)
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -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 (!allow_unsafe_interrupts)
-               return -EPERM;
+       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");
+               dev_warn(
+                       idev->dev,
+                       "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);

diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 3467cea795684c9f203f519ee7db7f88b59b4d47..e0ae72b9e67f86f89ed29ac29c6363f52821c776 100644 (file)
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -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;