From: Haitao Shan <maillists.shan@gmail.com>
To: Keir Fraser <keir.fraser@eu.citrix.com>,
Ian Jackson <Ian.Jackson@eu.citrix.com>,
Jan Beulich <jbeulich@novell.com>,
George Dunlap <George.Dunlap@eu.citrix.com>, Tim Deegan <Tim.De>
Subject: [PATCH] tools/ioemu: Fixing Security Hole in Qemu MSIX table access management
Date: Tue, 12 Jul 2011 13:24:59 +0800 [thread overview]
Message-ID: <CAFQ2Z+f+7ax3G63NgvghtjYX3ZucofVYQ6AOtMHaA+dCUT8+-g@mail.gmail.com> (raw)
[-- Attachment #1: Type: text/plain, Size: 9076 bytes --]
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@intel.com>
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 #2: fix_msix_sec_hole.patch --]
[-- Type: application/octet-stream, Size: 8340 bytes --]
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 #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
next reply other threads:[~2011-07-12 5:24 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-12 5:24 Haitao Shan [this message]
2011-07-12 7:05 ` [PATCH] tools/ioemu: Fixing Security Hole in Qemu MSIX table access management Jan Beulich
2011-07-12 9:30 ` Haitao Shan
2011-07-12 9:48 ` Jan Beulich
2011-07-12 13:32 ` Haitao Shan
2011-08-25 11:06 ` Ian Jackson
2011-08-25 11:55 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFQ2Z+f+7ax3G63NgvghtjYX3ZucofVYQ6AOtMHaA+dCUT8+-g@mail.gmail.com \
--to=maillists.shan@gmail.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=jbeulich@novell.com \
--cc=keir.fraser@eu.citrix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.