Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ankur Arora <ankur.a.arora@oracle.com>
To:     bbhushan2@marvell.com, alex.williamson@redhat.com
Cc:     linux-kernel@vger.kernel.org, sgoutham@marvell.com,
        ankur.a.arora@oracle.com, terminus@gmail.com
Subject: Re: vfio-pci: protect remap_pfn_range() from simultaneous calls
Date:   Wed, 20 Jan 2021 20:39:07 -0800
Message-Id: <20210121043907.76037-1-ankur.a.arora@oracle.com>
In-Reply-To: <MWHPR18MB1520D43B19368D8D84360248E3A31@MWHPR18MB1520.namprd18.prod.outlook.com>
References: <MWHPR18MB1520D43B19368D8D84360248E3A31@MWHPR18MB1520.namprd18.prod.outlook.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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