Messages in this thread Patch in this message | | | From | Ankur Arora <> | Subject | Re: vfio-pci: protect remap_pfn_range() from simultaneous calls | Date | Wed, 20 Jan 2021 20:39:07 -0800 |
| |
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 <ankur.a.arora@oracle.com> --- 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
| |