> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Thursday, November 24, 2022 3:48 AM
>
> 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;
> -
keep the blank line.
> 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
s/in/is/
> 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 {
this looks good to me:
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
