On Mon, Nov 21, 2022 at 09:59:28PM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 18, 2022 at 01:36:46PM -0700, Alex Williamson wrote:
> > On Fri, 18 Nov 2022 11:36:14 -0400
> > Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > > On Thu, Nov 17, 2022 at 01:14:51PM -0700, Alex Williamson wrote:
> > > > On Wed, 16 Nov 2022 17:05:29 -0400
> > > > Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> > > >
> > > > > This legacy module knob has become uAPI, when set on the vfio_iommu_type1
> > > > > it disables some security protections in the iommu drivers. Move the
> > > > > storage for this knob to vfio_main.c so that iommufd can access it too.
> > > > >
> > > > > The may need enhancing as we learn more about how necessary
> > > > > allow_unsafe_interrupts is in the current state of the world. If vfio
> > > > > container is disabled then this option will not be available to the user.
> > > > >
> > > > > Tested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > > > > Tested-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > > > > Tested-by: Lixiao Yang <lixiao.yang@xxxxxxxxx>
> > > > > Tested-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> > > > > Tested-by: Yu He <yu.he@xxxxxxxxx>
> > > > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/vfio/vfio.h | 2 ++
> > > > > drivers/vfio/vfio_iommu_type1.c | 5 ++---
> > > > > drivers/vfio/vfio_main.c | 3 +++
> > > > > 3 files changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > It's really quite trivial to convert to a vfio_iommu.ko module to host
> > > > a separate option for this. Half of the patch below is undoing what's
> > > > done here. Is your only concern with this approach that we use a few
> > > > KB more memory for the separate module?
> > >
> > > My main dislike is that it just seems arbitary to shunt iommufd
> > > support to a module when it is always required by vfio.ko. In general
> > > if you have a module that is only ever used by 1 other module, you
> > > should probably just combine them. It saves memory and simplifies
> > > operation (eg you don't have to unload a zoo of modules during
> > > development testing)
> >
> > These are all great reasons for why iommufd should host this option, as
> > it's fundamentally part of the DMA isolation of the device, which vfio
> > relies on iommufd to provide in this case.
>
> Fine, lets do that.
It looks like this:
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 07d4dcc0dbf5e1..6d088af776034b 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -9,6 +9,13 @@
#include "io_pagetable.h"
#include "iommufd_private.h"
+static bool allow_unsafe_interrupts;
+module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(
+ allow_unsafe_interrupts,
+ "Allow IOMMUFD to bind to devices even if the platform cannot isolate "
+ "the MSI interrupt window. Enabling this is a security weakness.");
+
/*
* A iommufd_device object represents the binding relationship between a
* consuming driver and the iommufd. These objects are created/destroyed by
@@ -127,8 +134,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
static int iommufd_device_setup_msi(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt,
- phys_addr_t sw_msi_start,
- unsigned int flags)
+ phys_addr_t sw_msi_start)
{
int rc;
@@ -174,12 +180,11 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
* historical compat with VFIO allow a module parameter to ignore the
* insecurity.
*/
- if (!(flags & IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT))
+ if (!allow_unsafe_interrupts)
return -EPERM;
-
dev_warn(
idev->dev,
- "Device interrupts cannot be isolated by the IOMMU, this platform in insecure. Use an \"allow_unsafe_interrupts\" module parameter to override\n");
+ "MSI interrupt window cannot be isolated by the IOMMU, this platform in insecure. Use the \"allow_unsafe_interrupts\" module parameter to override\n");
return 0;
}
@@ -195,8 +200,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,
- unsigned int flags)
+ struct iommufd_hw_pagetable *hwpt)
{
phys_addr_t sw_msi_start = 0;
int rc;
@@ -226,7 +230,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
if (rc)
goto out_unlock;
- rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start, flags);
+ rc = iommufd_device_setup_msi(idev, hwpt, sw_msi_start);
if (rc)
goto out_iova;
@@ -268,8 +272,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
* Automatic domain selection will never pick a manually created domain.
*/
static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
- struct iommufd_ioas *ioas,
- unsigned int flags)
+ struct iommufd_ioas *ioas)
{
struct iommufd_hw_pagetable *hwpt;
int rc;
@@ -284,7 +287,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
if (!hwpt->auto_domain)
continue;
- rc = iommufd_device_do_attach(idev, hwpt, flags);
+ rc = iommufd_device_do_attach(idev, hwpt);
/*
* -EINVAL means the domain is incompatible with the device.
@@ -303,7 +306,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
}
hwpt->auto_domain = true;
- rc = iommufd_device_do_attach(idev, hwpt, flags);
+ rc = iommufd_device_do_attach(idev, hwpt);
if (rc)
goto out_abort;
list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
@@ -324,7 +327,6 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
* @idev: device to attach
* @pt_id: Input a IOMMUFD_OBJ_IOAS, or IOMMUFD_OBJ_HW_PAGETABLE
* Output the IOMMUFD_OBJ_HW_PAGETABLE ID
- * @flags: Optional flags
*
* This connects the device to an iommu_domain, either automatically or manually
* selected. Once this completes the device could do DMA.
@@ -332,8 +334,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
* The caller should return the resulting pt_id back to userspace.
* This function is undone by calling iommufd_device_detach().
*/
-int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id,
- unsigned int flags)
+int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
{
struct iommufd_object *pt_obj;
int rc;
@@ -347,7 +348,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id,
struct iommufd_hw_pagetable *hwpt =
container_of(pt_obj, struct iommufd_hw_pagetable, obj);
- rc = iommufd_device_do_attach(idev, hwpt, flags);
+ rc = iommufd_device_do_attach(idev, hwpt);
if (rc)
goto out_put_pt_obj;
@@ -360,7 +361,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id,
struct iommufd_ioas *ioas =
container_of(pt_obj, struct iommufd_ioas, obj);
- rc = iommufd_device_auto_get_domain(idev, ioas, flags);
+ rc = iommufd_device_auto_get_domain(idev, ioas);
if (rc)
goto out_put_pt_obj;
break;
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 5a7ce4d9fbae0a..da50feb24b6e1d 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -108,12 +108,9 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_physical_unbind);
int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
{
- unsigned int flags = 0;
int rc;
- if (vfio_allow_unsafe_interrupts)
- flags |= IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT;
- rc = iommufd_device_attach(vdev->iommufd_device, pt_id, flags);
+ rc = iommufd_device_attach(vdev->iommufd_device, pt_id);
if (rc)
return rc;
vdev->iommufd_attached = true;
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 3378714a746274..ce5fe3fc493b4e 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -216,6 +216,4 @@ extern bool vfio_noiommu __read_mostly;
enum { vfio_noiommu = false };
#endif
-extern bool vfio_allow_unsafe_interrupts;
-
#endif
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 186e33a006d314..23c24fe98c00d4 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -44,8 +44,9 @@
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@xxxxxxxxxx>"
#define DRIVER_DESC "Type1 IOMMU driver for VFIO"
+static bool allow_unsafe_interrupts;
module_param_named(allow_unsafe_interrupts,
- vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
+ allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
MODULE_PARM_DESC(allow_unsafe_interrupts,
"Enable VFIO IOMMU support for on platforms without interrupt remapping support.");
@@ -2281,7 +2282,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP,
vfio_iommu_device_capable);
- if (!vfio_allow_unsafe_interrupts && !msi_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;
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 451a07eb702b34..593d45f43a16ba 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -52,9 +52,6 @@ static struct vfio {
struct ida device_ida;
} vfio;
-bool vfio_allow_unsafe_interrupts;
-EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts);
-
static DEFINE_XARRAY(vfio_device_set_xa);
static const struct file_operations vfio_group_fops;
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index bf2b3ea5f90fd2..9d1afd417215d0 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -21,11 +21,7 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
struct device *dev, u32 *id);
void iommufd_device_unbind(struct iommufd_device *idev);
-enum {
- IOMMUFD_ATTACH_FLAGS_ALLOW_UNSAFE_INTERRUPT = 1 << 0,
-};
-int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id,
- unsigned int flags);
+int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id);
void iommufd_device_detach(struct iommufd_device *idev);
struct iommufd_access_ops {
