From d6c55c0a20e5059abdde81713ddf6324a946eb3c Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Wed, 7 Dec 2022 16:44:43 -0400 Subject: [PATCH] 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 Reported-by: Eric Auger Signed-off-by: Jason Gunthorpe --- drivers/iommu/iommufd/device.c | 56 ++++++++++++++++-------------------- drivers/iommu/iommufd/io_pagetable.c | 24 +++++++++------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index dd2a415..d81f93a 100644 --- 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 3467cea..e0ae72b 100644 --- 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; -- 2.7.4