Re: [PATCH v2 0/4] Make the iommu driver no-snoop block feature consistent

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 07, 2022 at 07:02:03PM +0100, Robin Murphy wrote:
> On 2022-04-07 18:43, Jason Gunthorpe wrote:
> > On Thu, Apr 07, 2022 at 06:03:37PM +0100, Robin Murphy wrote:
> > > At a glance, this all looks about the right shape to me now, thanks!
> > 
> > Thanks!
> > 
> > > Ideally I'd hope patch #4 could go straight to device_iommu_capable() from
> > > my Thunderbolt series, but we can figure that out in a couple of weeks once
> > 
> > Yes, this does helps that because now the only iommu_capable call is
> > in a context where a device is available :)
> 
> Derp, of course I have *two* VFIO patches waiting, the other one touching
> the iommu_capable() calls (there's still IOMMU_CAP_INTR_REMAP, which, much
> as I hate it and would love to boot all that stuff over to
> drivers/irqchip,

Oh me too...

> it's not in my way so I'm leaving it be for now). I'll have to rebase that
> anyway, so merging this as-is is absolutely fine!

This might help your effort - after this series and this below there
are no 'bus' users of iommu_capable left at all.

From 55d72be40bc0a031711e318c8dd1cb60673d9eca Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@nvidia.com>
Date: Thu, 7 Apr 2022 16:00:50 -0300
Subject: [PATCH] vfio: Move the IOMMU_CAP_INTR_REMAP to a context with a
 struct device

This check is done to ensure that the device we want to use is fully
isolated and the platform does not allow the device's MemWr TLPs to
trigger unauthorized MSIs.

Instead of doing it in the container context where we only have a group,
move the check to open_device where we actually know the device.

This is still security safe as userspace cannot trigger an MemWr TLPs
without obtaining a device fd.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c             |  9 +++++++++
 drivers/vfio/vfio.h             |  1 +
 drivers/vfio/vfio_iommu_type1.c | 28 +++++++++++++++++-----------
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 9edad767cfdad3..8db5cea1dc1d75 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1355,6 +1355,15 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	if (IS_ERR(device))
 		return PTR_ERR(device);
 
+	/* Confirm this device is compatible with the container */
+	if (group->type == VFIO_IOMMU &&
+	    group->container->iommu_driver->ops->device_ok) {
+		ret = group->container->iommu_driver->ops->device_ok(
+			group->container->iommu_data, device->dev);
+		if (ret)
+			goto err_device_put;
+	}
+
 	if (!try_module_get(device->dev->driver->owner)) {
 		ret = -ENODEV;
 		goto err_device_put;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a6713022115155..3db60de71d42eb 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -66,6 +66,7 @@ struct vfio_iommu_driver_ops {
 						   struct iommu_group *group);
 	void		(*notify)(void *iommu_data,
 				  enum vfio_iommu_notify_type event);
+	int		(*device_ok)(void *iommu_data, struct device *device);
 };
 
 int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c13b9290e35759..5e966fb0ab9202 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2153,6 +2153,21 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
+static int vfio_iommu_device_ok(void *iommu_data, struct device *device)
+{
+	bool msi_remap;
+
+	msi_remap = irq_domain_check_msi_remap() ||
+		    iommu_capable(device->bus, IOMMU_CAP_INTR_REMAP);
+
+	if (!allow_unsafe_interrupts && !msi_remap) {
+		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
+			__func__);
+		return -EPERM;
+	}
+	return 0;
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 		struct iommu_group *iommu_group, enum vfio_group_type type)
 {
@@ -2160,7 +2175,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	struct vfio_iommu_group *group;
 	struct vfio_domain *domain, *d;
 	struct bus_type *bus = NULL;
-	bool resv_msi, msi_remap;
+	bool resv_msi;
 	phys_addr_t resv_msi_base = 0;
 	struct iommu_domain_geometry *geo;
 	LIST_HEAD(iova_copy);
@@ -2257,16 +2272,6 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = irq_domain_check_msi_remap() ||
-		    iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
-
-	if (!allow_unsafe_interrupts && !msi_remap) {
-		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
-		       __func__);
-		ret = -EPERM;
-		goto out_detach;
-	}
-
 	/*
 	 * If the IOMMU can block non-coherent operations (ie PCIe TLPs with
 	 * no-snoop set) then VFIO always turns this feature on because on Intel
@@ -3159,6 +3164,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
 	.open			= vfio_iommu_type1_open,
 	.release		= vfio_iommu_type1_release,
 	.ioctl			= vfio_iommu_type1_ioctl,
+	.device_ok		= vfio_iommu_device_ok,
 	.attach_group		= vfio_iommu_type1_attach_group,
 	.detach_group		= vfio_iommu_type1_detach_group,
 	.pin_pages		= vfio_iommu_type1_pin_pages,
-- 
2.35.1