Re: [PATCH 2/2] vfio: Introduce strict PFNMAP mappings

2020-05-19 Thread Jason Gunthorpe
On Thu, May 14, 2020 at 10:52:09AM -0600, Alex Williamson wrote:
>   vfio_unregister_iommu_driver(_noiommu_ops);
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 62ba6bd8a486..8d6286d89230 100644
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -61,6 +61,11 @@ module_param_named(dma_entry_limit, dma_entry_limit, uint, 
> 0644);
>  MODULE_PARM_DESC(dma_entry_limit,
>"Maximum number of user DMA mappings per container (65535).");
>  
> +static bool strict_mmio_maps = true;
> +module_param_named(strict_mmio_maps, strict_mmio_maps, bool, 0644);
> +MODULE_PARM_DESC(strict_mmio_maps,
> +  "Restrict DMA mappings of MMIO to those provided by vfio bus 
> drivers supporting invalidation (true).");
> +

This should probably explain that 'false' allows some kind of security
issue and maybe taint the kernel?

Do you think there is a reason to have this anyhow?

Jason


[PATCH 2/2] vfio: Introduce strict PFNMAP mappings

2020-05-14 Thread Alex Williamson
We can't pin PFNMAP IOMMU mappings like we can standard page-backed
mappings, therefore without an invalidation mechanism we can't know
if we should have revoked a user's mapping.  Now that we have an
invalidation callback mechanism we can create an interface for vfio
bus drivers to indicate their support for invalidation by registering
supported vm_ops functions with vfio-core.  A vfio IOMMU backend
driver can then test a vma against the registered vm_ops with this
support to determine whether to allow such a mapping.  The type1
backend then adopts a new 'strict_mmio_maps' module option, enabled
by default, restricting IOMMU mapping of PFNMAP vmas to only those
supporting invalidation callbacks.  vfio-pci is updated to register
vfio_pci_mmap_ops as supporting this feature.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/pci/vfio_pci.c |7 
 drivers/vfio/vfio.c |   62 +++
 drivers/vfio/vfio_iommu_type1.c |9 +-
 include/linux/vfio.h|4 +++
 4 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 100fe5f6bc22..dbfe6a11aa74 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -2281,6 +2281,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device 
*vdev)
 
 static void __exit vfio_pci_cleanup(void)
 {
+   vfio_unregister_vma_inv_ops(_pci_mmap_ops);
pci_unregister_driver(_pci_driver);
vfio_pci_uninit_perm_bits();
 }
@@ -2340,10 +2341,16 @@ static int __init vfio_pci_init(void)
if (ret)
goto out_driver;
 
+   ret = vfio_register_vma_inv_ops(_pci_mmap_ops);
+   if (ret)
+   goto out_inv_ops;
+
vfio_pci_fill_ids();
 
return 0;
 
+out_inv_ops:
+   pci_unregister_driver(_pci_driver);
 out_driver:
vfio_pci_uninit_perm_bits();
return ret;
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0fff057b7cd9..0f0a9d3b38aa 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -47,6 +47,8 @@ static struct vfio {
struct cdev group_cdev;
dev_t   group_devt;
wait_queue_head_t   release_q;
+   struct list_headvma_inv_ops_list;
+   struct mutexvma_inv_ops_lock;
 } vfio;
 
 struct vfio_iommu_driver {
@@ -98,6 +100,11 @@ struct vfio_device {
void*device_data;
 };
 
+struct vfio_vma_inv_ops {
+   const struct vm_operations_struct   *ops;
+   struct list_headops_next;
+};
+
 #ifdef CONFIG_VFIO_NOIOMMU
 static bool noiommu __read_mostly;
 module_param_named(enable_unsafe_noiommu_mode,
@@ -2332,6 +2339,58 @@ int vfio_unregister_notifier(struct device *dev, enum 
vfio_notify_type type,
 }
 EXPORT_SYMBOL(vfio_unregister_notifier);
 
+int vfio_register_vma_inv_ops(const struct vm_operations_struct *ops)
+{
+   struct vfio_vma_inv_ops *inv_ops;
+
+   inv_ops = kmalloc(sizeof(*inv_ops), GFP_KERNEL);
+   if (!inv_ops)
+   return -ENOMEM;
+
+   inv_ops->ops = ops;
+
+   mutex_lock(_inv_ops_lock);
+   list_add(_ops->ops_next, _inv_ops_list);
+   mutex_unlock(_inv_ops_lock);
+
+   return 0;
+}
+EXPORT_SYMBOL(vfio_register_vma_inv_ops);
+
+void vfio_unregister_vma_inv_ops(const struct vm_operations_struct *ops)
+{
+   struct vfio_vma_inv_ops *inv_ops;
+
+   mutex_lock(_inv_ops_lock);
+   list_for_each_entry(inv_ops, _inv_ops_list, ops_next) {
+   if (inv_ops->ops == ops) {
+   list_del(_ops->ops_next);
+   kfree(inv_ops);
+   break;
+   }
+   }
+   mutex_unlock(_inv_ops_lock);
+}
+EXPORT_SYMBOL(vfio_unregister_vma_inv_ops);
+
+bool vfio_vma_has_inv_ops(struct vm_area_struct *vma)
+{
+   struct vfio_vma_inv_ops *inv_ops;
+   bool ret = false;
+
+   mutex_lock(_inv_ops_lock);
+   list_for_each_entry(inv_ops, _inv_ops_list, ops_next) {
+   if (inv_ops->ops == vma->vm_ops) {
+   ret = true;
+   break;
+   }
+   }
+   mutex_unlock(_inv_ops_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(vfio_vma_has_inv_ops);
+
 /**
  * Module/class support
  */
@@ -2355,8 +2414,10 @@ static int __init vfio_init(void)
idr_init(_idr);
mutex_init(_lock);
mutex_init(_drivers_lock);
+   mutex_init(_inv_ops_lock);
INIT_LIST_HEAD(_list);
INIT_LIST_HEAD(_drivers_list);
+   INIT_LIST_HEAD(_inv_ops_list);
init_waitqueue_head(_q);
 
ret = misc_register(_dev);
@@ -2403,6 +2464,7 @@ static int __init vfio_init(void)
 static void __exit vfio_cleanup(void)
 {
WARN_ON(!list_empty(_list));
+   WARN_ON(!list_empty(_inv_ops_list));
 
 #ifdef CONFIG_VFIO_NOIOMMU