Hi Bharat,
So I don't have a final patch for you, but can you test the one below
the scissors mark? (The patch is correct, but I'm not happy that it
introduces a new lock.)
Ankur
-- >8 --
Date: Thu, 21 Jan 2021 00:04:36 +0000
Subject: vfio-pci: protect io_remap_pfn_range() from simultaneous calls
A fix for CVE-2020-12888 changed the mmap to be dynamic so it gets
zapped out and faulted back in based on the state of the PCI_COMMAND
register.
The original code flow was:
vfio_iommu_type1::vfio_device_mmap()
vfio_pci::vfio_pci_mmap()
remap_pfn_range(vma->vm_start .. vma->vm_end);
vfio_iommu_type1::vfio_iommu_type1_ioctl(VFIO_PIN_MAP_DMA)
get_user_pages_remote()
iommu_map()
Which became:
vfio_iommu_type1::vfio_device_mmap()
vfio_pci::vfio_pci_mmap()
/* Setup vm->vm_ops */
vfio_iommu_type1::vfio_iommu_type1_ioctl(VFIO_PIN_MAP_DMA)
get_user_pages_remote()
follow_fault_pfn(vma, vaddr); /* For missing pages */
fixup_user_fault()
handle_mm_fault()
vfio_pci::vfio_pci_mmap_fault()
io_remap_pfn_range(vma->vm_start .. vma->vm_end);
iommu_map()
With this, simultaneous calls to VFIO_PIN_MAP_DMA for an overlapping
region, would mean potentially multiple calls to io_remap_pfn_range()
-- each of which try to remap the full extent.
This ends up hitting BUG_ON(!pte_none(*pte)) in remap_pte_range()
because we are mapping an already mapped pte.
The fix is to elide calls to io_remap_pfn_range() if the VMA is already
mapped.
Fixes: abafbc551fdd ("vfio-pci: Invalidate mmaps and block MMIO access
on disabled memory")
Signed-off-by: Ankur Arora <[email protected]>
---
drivers/vfio/pci/vfio_pci.c | 48 ++++++++++++++++++++++++++++++++++---
drivers/vfio/pci/vfio_pci_private.h | 2 ++
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 706de3ef94bb..db7a2a716f51 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -64,6 +64,11 @@ static bool disable_denylist;
module_param(disable_denylist, bool, 0444);
MODULE_PARM_DESC(disable_denylist, "Disable use of device denylist. Disabling
the denylist allows binding to devices with known errata that may lead to
exploitable stability or security issues when accessed by untrusted users.");
+struct vdev_vma_priv {
+ struct vfio_pci_device *vdev;
+ bool vma_mapped;
+};
+
static inline bool vfio_vga_disabled(void)
{
#ifdef CONFIG_VFIO_PCI_VGA
@@ -1527,15 +1532,20 @@ static int vfio_pci_zap_and_vma_lock(struct
vfio_pci_device *vdev, bool try)
list_for_each_entry_safe(mmap_vma, tmp,
&vdev->vma_list, vma_next) {
struct vm_area_struct *vma = mmap_vma->vma;
+ struct vdev_vma_priv *p;
if (vma->vm_mm != mm)
continue;
list_del(&mmap_vma->vma_next);
kfree(mmap_vma);
+ p = vma->vm_private_data;
+ mutex_lock(&vdev->map_lock);
zap_vma_ptes(vma, vma->vm_start,
vma->vm_end - vma->vm_start);
+ p->vma_mapped = false;
+ mutex_unlock(&vdev->map_lock);
}
mutex_unlock(&vdev->vma_lock);
mmap_read_unlock(mm);
@@ -1591,12 +1601,19 @@ static int __vfio_pci_add_vma(struct vfio_pci_device
*vdev,
*/
static void vfio_pci_mmap_open(struct vm_area_struct *vma)
{
+ struct vdev_vma_priv *p = vma->vm_private_data;
+ struct vfio_pci_device *vdev = p->vdev;
+
+ mutex_lock(&vdev->map_lock);
+ p->vma_mapped = false;
zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
+ mutex_unlock(&vdev->map_lock);
}
static void vfio_pci_mmap_close(struct vm_area_struct *vma)
{
- struct vfio_pci_device *vdev = vma->vm_private_data;
+ struct vdev_vma_priv *p = vma->vm_private_data;
+ struct vfio_pci_device *vdev = p->vdev;
struct vfio_pci_mmap_vma *mmap_vma;
mutex_lock(&vdev->vma_lock);
@@ -1604,6 +1621,7 @@ static void vfio_pci_mmap_close(struct vm_area_struct
*vma)
if (mmap_vma->vma == vma) {
list_del(&mmap_vma->vma_next);
kfree(mmap_vma);
+ kfree(p);
break;
}
}
@@ -1613,7 +1631,8 @@ static void vfio_pci_mmap_close(struct vm_area_struct
*vma)
static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
{
struct vm_area_struct *vma = vmf->vma;
- struct vfio_pci_device *vdev = vma->vm_private_data;
+ struct vdev_vma_priv *p = vma->vm_private_data;
+ struct vfio_pci_device *vdev = p->vdev;
vm_fault_t ret = VM_FAULT_NOPAGE;
mutex_lock(&vdev->vma_lock);
@@ -1633,10 +1652,24 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fault
*vmf)
mutex_unlock(&vdev->vma_lock);
+ /*
+ * The vdev->map_lock in vfio_pci_zap_and_vma_lock() nests
+ * inside the vdev->vma_lock but doesn't depend on that for
+ * protection of the VMA.
+ * So take vdev->map_lock after releasing vdev->vma_lock.
+ */
+ mutex_lock(&vdev->map_lock);
+ if (p->vma_mapped)
+ goto unlock_out;
+
if (io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
vma->vm_end - vma->vm_start, vma->vm_page_prot))
ret = VM_FAULT_SIGBUS;
+ else
+ p->vma_mapped = true;
+unlock_out:
+ mutex_unlock(&vdev->map_lock);
up_out:
up_read(&vdev->memory_lock);
return ret;
@@ -1654,6 +1687,7 @@ static int vfio_pci_mmap(void *device_data, struct
vm_area_struct *vma)
struct pci_dev *pdev = vdev->pdev;
unsigned int index;
u64 phys_len, req_len, pgoff, req_start;
+ struct vdev_vma_priv *priv;
int ret;
index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
@@ -1702,7 +1736,14 @@ static int vfio_pci_mmap(void *device_data, struct
vm_area_struct *vma)
}
}
- vma->vm_private_data = vdev;
+ priv = kzalloc(sizeof(struct vdev_vma_priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->vdev = vdev;
+ priv->vma_mapped = false;
+
+ vma->vm_private_data = priv;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
vma->vm_pgoff = (pci_resource_start(pdev, index) >> PAGE_SHIFT) + pgoff;
@@ -1967,6 +2008,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
INIT_LIST_HEAD(&vdev->dummy_resources_list);
INIT_LIST_HEAD(&vdev->ioeventfds_list);
mutex_init(&vdev->vma_lock);
+ mutex_init(&vdev->map_lock);
INIT_LIST_HEAD(&vdev->vma_list);
init_rwsem(&vdev->memory_lock);
diff --git a/drivers/vfio/pci/vfio_pci_private.h
b/drivers/vfio/pci/vfio_pci_private.h
index 5c90e560c5c7..f3010c49b06c 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -142,6 +142,8 @@ struct vfio_pci_device {
struct mutex vma_lock;
struct list_head vma_list;
struct rw_semaphore memory_lock;
+ /* Protects VMA against simultaneous remaps. */
+ struct mutex map_lock;
};
#define is_intx(vdev) (vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX)
--
2.9.3
