[Xen-devel] [PATCH] tools/ioemu: Fixing Security Hole in Qemu MSIX table access management

[Haitao Shan <maillists.shan@xxxxxxxxx>]

Hi,

As reported by Jan, current Qemu does not handle MSIX table mapping properly.

Details:

MSI-X table resides in one of the physical BARs. When Qemu handles
guest's changes to BAR register (within which, MSI-X table resides),
Qemu first allows access of the whole BAR MMIO ranges and then removes
those of MSI-X. There is a small window here. It is possible that on a
SMP guests one vcpu could have access to the physical MSI-X
configurations when another vcpu is writing BAR registers.

The patch fixes this issue by first producing the valid MMIO ranges by
removing MSI-X table's range from the whole BAR mmio range and later
passing these ranges to Xen.

Please have a review, thanks!

Signed-off-by:    Shan Haitao <haitao.shan@xxxxxxxxx>

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 9c5620d..b9c2f32 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -92,6 +92,7 @@

 #include <unistd.h>
 #include <sys/ioctl.h>
+#include <assert.h>

 extern int gfx_passthru;
 int igd_passthru = 0;
@@ -1103,6 +1104,7 @@ static void pt_iomem_map(PCIDevice *d, int i,
uint32_t e_phys, uint32_t e_size,
 {
     struct pt_dev *assigned_device  = (struct pt_dev *)d;
     uint32_t old_ebase = assigned_device->bases[i].e_physbase;
+    uint32_t msix_last_pfn = 0, bar_last_pfn = 0;
     int first_map = ( assigned_device->bases[i].e_size == 0 );
     int ret = 0;

@@ -1118,39 +1120,124 @@ static void pt_iomem_map(PCIDevice *d, int i,
uint32_t e_phys, uint32_t e_size,

     if ( !first_map && old_ebase != -1 )
     {
-        add_msix_mapping(assigned_device, i);
-        /* Remove old mapping */
-        ret = xc_domain_memory_mapping(xc_handle, domid,
+        if ( has_msix_mapping(assigned_device, i) )
+        {
+            msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 +
+                  assigned_device->msix->total_entries * 16) >>  XC_PAGE_SHIFT;
+            bar_last_pfn = (old_ebase + e_size - 1) >> XC_PAGE_SHIFT;
+
+            if ( assigned_device->msix->table_off )
+            {
+                       ret = xc_domain_memory_mapping(xc_handle, domid,
+                    old_ebase >> XC_PAGE_SHIFT,
+                    assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
+                    (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT)
+                    - (old_ebase >> XC_PAGE_SHIFT),
+                    DPCI_REMOVE_MAPPING);
+                if ( ret != 0 )
+                {
+                    PT_LOG("Error: remove old mapping failed!\n");
+                    return;
+                }
+            }
+            if ( msix_last_pfn != bar_last_pfn )
+            {
+                assert(msix_last_pfn < bar_last_pfn);
+                       ret = xc_domain_memory_mapping(xc_handle, domid,
+                    msix_last_pfn + 1,
+                    (assigned_device->bases[i].access.maddr +
+                     assigned_device->msix->table_off +
+                     assigned_device->msix->total_entries * 16 +
+                     XC_PAGE_SIZE -1) >>  XC_PAGE_SHIFT,
+                    bar_last_pfn - msix_last_pfn,
+                    DPCI_REMOVE_MAPPING);
+                if ( ret != 0 )
+                {
+                    PT_LOG("Error: remove old mapping failed!\n");
+                    return;
+                }
+            }
+        }
+        else
+        {
+                   /* Remove old mapping */
+                   ret = xc_domain_memory_mapping(xc_handle, domid,
                 old_ebase >> XC_PAGE_SHIFT,
                 assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
                 (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
                 DPCI_REMOVE_MAPPING);
-        if ( ret != 0 )
-        {
-            PT_LOG("Error: remove old mapping failed!\n");
-            return;
+            if ( ret != 0 )
+            {
+                PT_LOG("Error: remove old mapping failed!\n");
+                return;
+            }
         }
     }

     /* map only valid guest address */
     if (e_phys != -1)
     {
-        /* Create new mapping */
-        ret = xc_domain_memory_mapping(xc_handle, domid,
+        if ( has_msix_mapping(assigned_device, i) )
+               {
+            assigned_device->msix->mmio_base_addr =
+                assigned_device->bases[i].e_physbase
+                + assigned_device->msix->table_off;
+
+            msix_last_pfn = (assigned_device->msix->mmio_base_addr - 1 +
+                  assigned_device->msix->total_entries * 16) >>  XC_PAGE_SHIFT;
+            bar_last_pfn = (e_phys + e_size - 1) >> XC_PAGE_SHIFT;
+
+            cpu_register_physical_memory(assigned_device->msix->mmio_base_addr,
+                                 assigned_device->msix->total_entries * 16,
+                                 assigned_device->msix->mmio_index);
+
+            if ( assigned_device->msix->table_off )
+            {
+                       ret = xc_domain_memory_mapping(xc_handle, domid,
+                    assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT,
+                    assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
+                    (assigned_device->msix->mmio_base_addr >> XC_PAGE_SHIFT)
+                    - (assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT),
+                    DPCI_ADD_MAPPING);
+                if ( ret != 0 )
+                {
+                    PT_LOG("Error: remove old mapping failed!\n");
+                    return;
+                }
+            }
+            if ( msix_last_pfn != bar_last_pfn )
+            {
+                assert(msix_last_pfn < bar_last_pfn);
+                       ret = xc_domain_memory_mapping(xc_handle, domid,
+                    msix_last_pfn + 1,
+                    (assigned_device->bases[i].access.maddr +
+                     assigned_device->msix->table_off +
+                     assigned_device->msix->total_entries * 16 +
+                     XC_PAGE_SIZE -1) >>  XC_PAGE_SHIFT,
+                    bar_last_pfn - msix_last_pfn,
+                    DPCI_ADD_MAPPING);
+                if ( ret != 0 )
+                {
+                    PT_LOG("Error: remove old mapping failed!\n");
+                    return;
+                }
+            }
+               }
+               else
+        {
+                       /* Create new mapping */
+                       ret = xc_domain_memory_mapping(xc_handle, domid,
                 assigned_device->bases[i].e_physbase >> XC_PAGE_SHIFT,
                 assigned_device->bases[i].access.maddr >> XC_PAGE_SHIFT,
                 (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
                 DPCI_ADD_MAPPING);

-        if ( ret != 0 )
-        {
-            PT_LOG("Error: create new mapping failed!\n");
+            if ( ret != 0 )
+            {
+                PT_LOG("Error: create new mapping failed!\n");
+            }
         }

-        ret = remove_msix_mapping(assigned_device, i);
-        if ( ret != 0 )
-            PT_LOG("Error: remove MSI-X mmio mapping failed!\n");
-
         if ( old_ebase != e_phys && old_ebase != -1 )
             pt_msix_update_remap(assigned_device, i);
     }
diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 71fa6f0..1fbebd4 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -528,39 +528,12 @@ static CPUReadMemoryFunc *pci_msix_read[] = {
     pci_msix_readl
 };

-int add_msix_mapping(struct pt_dev *dev, int bar_index)
+int has_msix_mapping(struct pt_dev *dev, int bar_index)
 {
     if ( !(dev->msix && dev->msix->bar_index == bar_index) )
         return 0;

-    return xc_domain_memory_mapping(xc_handle, domid,
-                dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-                (dev->bases[bar_index].access.maddr
-                + dev->msix->table_off) >> XC_PAGE_SHIFT,
-                (dev->msix->total_entries * 16
-                + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
-                DPCI_ADD_MAPPING);
-}
-
-int remove_msix_mapping(struct pt_dev *dev, int bar_index)
-{
-    if ( !(dev->msix && dev->msix->bar_index == bar_index) )
-        return 0;
-
-    dev->msix->mmio_base_addr = dev->bases[bar_index].e_physbase
-                                + dev->msix->table_off;
-
-    cpu_register_physical_memory(dev->msix->mmio_base_addr,
-                                 dev->msix->total_entries * 16,
-                                 dev->msix->mmio_index);
-
-    return xc_domain_memory_mapping(xc_handle, domid,
-                dev->msix->mmio_base_addr >> XC_PAGE_SHIFT,
-                (dev->bases[bar_index].access.maddr
-                + dev->msix->table_off) >> XC_PAGE_SHIFT,
-                (dev->msix->total_entries * 16
-                + XC_PAGE_SIZE -1) >> XC_PAGE_SHIFT,
-                DPCI_REMOVE_MAPPING);
+       return 1;
 }

 int pt_msix_init(struct pt_dev *dev, int pos)
diff --git a/hw/pt-msi.h b/hw/pt-msi.h
index 9664f89..2dc1720 100644
--- a/hw/pt-msi.h
+++ b/hw/pt-msi.h
@@ -107,10 +107,7 @@ void
 pt_msix_disable(struct pt_dev *dev);

 int
-remove_msix_mapping(struct pt_dev *dev, int bar_index);
-
-int
-add_msix_mapping(struct pt_dev *dev, int bar_index);
+has_msix_mapping(struct pt_dev *dev, int bar_index);

 int
 pt_msix_init(struct pt_dev *dev, int pos);

Attachment: fix_msix_sec_hole.patch
Description: Binary data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel