[Qemu-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Qemu-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Stefano Stabellini]

Hi all,

the following is a collection of QEMU security fixes for PCI Passthrough
on Xen. Non-Xen usages of QEMU are unaffected.

Although the CVEs have already been made public, given the large amount
of changes, I decided not to send a pull request without giving a chance
to the QEMU community to comment on the patches first.

Each patch has a detail description of what is trying to fix. You can
also cross-reference the CVE numbers.



Jan Beulich (11):
      xen: properly gate host writes of modified PCI CFG contents
      xen: don't allow guest to control MSI mask register
      xen/MSI-X: limit error messages
      xen/MSI: don't open-code pass-through of enable bit modifications
      xen/pt: consolidate PM capability emu_mask
      xen/pt: correctly handle PM status bit
      xen/pt: split out calculation of throughable mask in PCI config space handling
      xen/pt: mark all PCIe capability bits read-only
      xen/pt: mark reserved bits in PCI config space fields
      xen/pt: add a few PCI config space field descriptions
      xen/pt: unknown PCI config space fields should be read-only

 hw/pci/msi.c                |    4 -
 hw/xen/xen_pt.c             |   51 +++++++++-
 hw/xen/xen_pt.h             |    7 +-
 hw/xen/xen_pt_config_init.c |  235 ++++++++++++++++++++++++++++---------------
 hw/xen/xen_pt_msi.c         |   12 ++-
 include/hw/pci/pci_regs.h   |    2 +
 6 files changed, 217 insertions(+), 94 deletions(-)


Cheers,

Stefano

[Qemu-devel] [PATCH 01/11] xen: properly gate host writes of modified PCI CFG contents

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

The old logic didn't work as intended when an access spanned multiple
fields (for example a 32-bit access to the location of the MSI Message
Data field with the high 16 bits not being covered by any known field).
Remove it and derive which fields not to write to from the accessed
fields' emulation masks: When they're all ones, there's no point in
doing any host write.

This fixes a secondary issue at once: We obviously shouldn't make any
host write attempt when already the host read failed.

This is XSA-128.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt.c             |   25 +++++++++++++++++++++----
 hw/xen/xen_pt.h             |    2 --
 hw/xen/xen_pt_config_init.c |    4 ----
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index d095c08..8923582 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -234,7 +234,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     int index = 0;
     XenPTRegGroup *reg_grp_entry = NULL;
     int rc = 0;
-    uint32_t read_val = 0;
+    uint32_t read_val = 0, wb_mask;
     int emul_len = 0;
     XenPTReg *reg_entry = NULL;
     uint32_t find_addr = addr;
@@ -271,6 +271,9 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     if (rc < 0) {
         XEN_PT_ERR(d, "pci_read_block failed. return value: %d.\n", rc);
         memset(&read_val, 0xff, len);
+        wb_mask = 0;
+    } else {
+        wb_mask = 0xFFFFFFFF >> ((4 - len) << 3);
     }
 
     /* pass directly to the real device for passthrough type register group */
@@ -298,6 +301,11 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
 
             valid_mask <<= (find_addr - real_offset) << 3;
             ptr_val = (uint8_t *)&val + (real_offset & 3);
+            if (reg->emu_mask == (0xFFFFFFFF >> ((4 - reg->size) << 3))) {
+                wb_mask &= ~((reg->emu_mask
+                              >> ((find_addr - real_offset) << 3))
+                             << ((len - emul_len) << 3));
+            }
 
             /* do emulation based on register size */
             switch (reg->size) {
@@ -350,10 +358,19 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     memory_region_transaction_commit();
 
 out:
-    if (!(reg && reg->no_wb)) {
+    for (index = 0; wb_mask; index += len) {
         /* unknown regs are passed through */
-        rc = xen_host_pci_set_block(&s->real_device, addr,
-                                    (uint8_t *)&val, len);
+        while (!(wb_mask & 0xff)) {
+            index++;
+            wb_mask >>= 8;
+        }
+        len = 0;
+        do {
+            len++;
+            wb_mask >>= 8;
+        } while (wb_mask & 0xff);
+        rc = xen_host_pci_set_block(&s->real_device, addr + index,
+                                    (uint8_t *)&val + index, len);
 
         if (rc < 0) {
             XEN_PT_ERR(d, "pci_write_block failed. return value: %d.\n", rc);
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 942dc60..52ceb85 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -105,8 +105,6 @@ struct XenPTRegInfo {
     uint32_t ro_mask;
     /* reg emulate field mask (ON:emu, OFF:passthrough) */
     uint32_t emu_mask;
-    /* no write back allowed */
-    uint32_t no_wb;
     xen_pt_conf_reg_init init;
     /* read/write function pointer
      * for double_word/word/byte size */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 95a51db..dae0519 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1279,7 +1279,6 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .init_val   = 0x00000000,
         .ro_mask    = 0x00000003,
         .emu_mask   = 0xFFFFFFFF,
-        .no_wb      = 1,
         .init       = xen_pt_common_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
         .u.dw.write = xen_pt_msgaddr32_reg_write,
@@ -1291,7 +1290,6 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .init_val   = 0x00000000,
         .ro_mask    = 0x00000000,
         .emu_mask   = 0xFFFFFFFF,
-        .no_wb      = 1,
         .init       = xen_pt_msgaddr64_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
         .u.dw.write = xen_pt_msgaddr64_reg_write,
@@ -1303,7 +1301,6 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .init_val   = 0x0000,
         .ro_mask    = 0x0000,
         .emu_mask   = 0xFFFF,
-        .no_wb      = 1,
         .init       = xen_pt_msgdata_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_msgdata_reg_write,
@@ -1315,7 +1312,6 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .init_val   = 0x0000,
         .ro_mask    = 0x0000,
         .emu_mask   = 0xFFFF,
-        .no_wb      = 1,
         .init       = xen_pt_msgdata_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_msgdata_reg_write,
-- 
1.7.10.4

[Qemu-devel] [PATCH 02/11] xen: don't allow guest to control MSI mask register

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

It's being used by the hypervisor. For now simply mimic a device not
capable of masking, and fully emulate any accesses a guest may issue
nevertheless as simple reads/writes without side effects.

This is XSA-129.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/pci/msi.c                |    4 --
 hw/xen/xen_pt_config_init.c |   98 ++++++++++++++++++++++++++++++++++++++-----
 include/hw/pci/pci_regs.h   |    2 +
 3 files changed, 90 insertions(+), 14 deletions(-)

diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index c111dba..f9c0484 100644
--- a/hw/pci/msi.c
+++ b/hw/pci/msi.c
@@ -21,10 +21,6 @@
 #include "hw/pci/msi.h"
 #include "qemu/range.h"
 
-/* Eventually those constants should go to Linux pci_regs.h */
-#define PCI_MSI_PENDING_32      0x10
-#define PCI_MSI_PENDING_64      0x14
-
 /* PCI_MSI_ADDRESS_LO */
 #define PCI_MSI_ADDRESS_LO_MASK         (~0x3)
 
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index dae0519..68b8f22 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1016,13 +1016,9 @@ static XenPTRegInfo xen_pt_emu_reg_pm[] = {
  */
 
 /* Helper */
-static bool xen_pt_msgdata_check_type(uint32_t offset, uint16_t flags)
-{
-    /* check the offset whether matches the type or not */
-    bool is_32 = (offset == PCI_MSI_DATA_32) && !(flags & PCI_MSI_FLAGS_64BIT);
-    bool is_64 = (offset == PCI_MSI_DATA_64) &&  (flags & PCI_MSI_FLAGS_64BIT);
-    return is_32 || is_64;
-}
+#define xen_pt_msi_check_type(offset, flags, what) \
+        ((offset) == ((flags) & PCI_MSI_FLAGS_64BIT ? \
+                      PCI_MSI_##what##_64 : PCI_MSI_##what##_32))
 
 /* Message Control register */
 static int xen_pt_msgctrl_reg_init(XenPCIPassthroughState *s,
@@ -1134,7 +1130,45 @@ static int xen_pt_msgdata_reg_init(XenPCIPassthroughState *s,
     uint32_t offset = reg->offset;
 
     /* check the offset whether matches the type or not */
-    if (xen_pt_msgdata_check_type(offset, flags)) {
+    if (xen_pt_msi_check_type(offset, flags, DATA)) {
+        *data = reg->init_val;
+    } else {
+        *data = XEN_PT_INVALID_REG;
+    }
+    return 0;
+}
+
+/* this function will be called twice (for 32 bit and 64 bit type) */
+/* initialize Mask register */
+static int xen_pt_mask_reg_init(XenPCIPassthroughState *s,
+                                XenPTRegInfo *reg, uint32_t real_offset,
+                                uint32_t *data)
+{
+    uint32_t flags = s->msi->flags;
+
+    /* check the offset whether matches the type or not */
+    if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
+        *data = XEN_PT_INVALID_REG;
+    } else if (xen_pt_msi_check_type(reg->offset, flags, MASK)) {
+        *data = reg->init_val;
+    } else {
+        *data = XEN_PT_INVALID_REG;
+    }
+    return 0;
+}
+
+/* this function will be called twice (for 32 bit and 64 bit type) */
+/* initialize Pending register */
+static int xen_pt_pending_reg_init(XenPCIPassthroughState *s,
+                                   XenPTRegInfo *reg, uint32_t real_offset,
+                                   uint32_t *data)
+{
+    uint32_t flags = s->msi->flags;
+
+    /* check the offset whether matches the type or not */
+    if (!(flags & PCI_MSI_FLAGS_MASKBIT)) {
+        *data = XEN_PT_INVALID_REG;
+    } else if (xen_pt_msi_check_type(reg->offset, flags, PENDING)) {
         *data = reg->init_val;
     } else {
         *data = XEN_PT_INVALID_REG;
@@ -1222,7 +1256,7 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     uint32_t offset = reg->offset;
 
     /* check the offset whether matches the type or not */
-    if (!xen_pt_msgdata_check_type(offset, msi->flags)) {
+    if (!xen_pt_msi_check_type(offset, msi->flags, DATA)) {
         /* exit I/O emulator */
         XEN_PT_ERR(&s->dev, "the offset does not match the 32/64 bit type!\n");
         return -1;
@@ -1267,7 +1301,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .size       = 2,
         .init_val   = 0x0000,
         .ro_mask    = 0xFF8E,
-        .emu_mask   = 0x007F,
+        .emu_mask   = 0x017F,
         .init       = xen_pt_msgctrl_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_msgctrl_reg_write,
@@ -1316,6 +1350,50 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_msgdata_reg_write,
     },
+    /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
+    {
+        .offset     = PCI_MSI_MASK_32,
+        .size       = 4,
+        .init_val   = 0x00000000,
+        .ro_mask    = 0xFFFFFFFF,
+        .emu_mask   = 0xFFFFFFFF,
+        .init       = xen_pt_mask_reg_init,
+        .u.dw.read  = xen_pt_long_reg_read,
+        .u.dw.write = xen_pt_long_reg_write,
+    },
+    /* Mask reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
+    {
+        .offset     = PCI_MSI_MASK_64,
+        .size       = 4,
+        .init_val   = 0x00000000,
+        .ro_mask    = 0xFFFFFFFF,
+        .emu_mask   = 0xFFFFFFFF,
+        .init       = xen_pt_mask_reg_init,
+        .u.dw.read  = xen_pt_long_reg_read,
+        .u.dw.write = xen_pt_long_reg_write,
+    },
+    /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 32-bit devices) */
+    {
+        .offset     = PCI_MSI_MASK_32 + 4,
+        .size       = 4,
+        .init_val   = 0x00000000,
+        .ro_mask    = 0xFFFFFFFF,
+        .emu_mask   = 0x00000000,
+        .init       = xen_pt_pending_reg_init,
+        .u.dw.read  = xen_pt_long_reg_read,
+        .u.dw.write = xen_pt_long_reg_write,
+    },
+    /* Pending reg (if PCI_MSI_FLAGS_MASKBIT set, for 64-bit devices) */
+    {
+        .offset     = PCI_MSI_MASK_64 + 4,
+        .size       = 4,
+        .init_val   = 0x00000000,
+        .ro_mask    = 0xFFFFFFFF,
+        .emu_mask   = 0x00000000,
+        .init       = xen_pt_pending_reg_init,
+        .u.dw.read  = xen_pt_long_reg_read,
+        .u.dw.write = xen_pt_long_reg_write,
+    },
     {
         .size = 0,
     },
diff --git a/include/hw/pci/pci_regs.h b/include/hw/pci/pci_regs.h
index 56a404b..57e8c80 100644
--- a/include/hw/pci/pci_regs.h
+++ b/include/hw/pci/pci_regs.h
@@ -298,8 +298,10 @@
 #define PCI_MSI_ADDRESS_HI	8	/* Upper 32 bits (if PCI_MSI_FLAGS_64BIT set) */
 #define PCI_MSI_DATA_32		8	/* 16 bits of data for 32-bit devices */
 #define PCI_MSI_MASK_32		12	/* Mask bits register for 32-bit devices */
+#define PCI_MSI_PENDING_32	16	/* Pending bits register for 32-bit devices */
 #define PCI_MSI_DATA_64		12	/* 16 bits of data for 64-bit devices */
 #define PCI_MSI_MASK_64		16	/* Mask bits register for 64-bit devices */
+#define PCI_MSI_PENDING_64	20	/* Pending bits register for 32-bit devices */
 
 /* MSI-X registers */
 #define PCI_MSIX_FLAGS		2
-- 
1.7.10.4

[Qemu-devel] [PATCH 03/11] xen/MSI-X: limit error messages

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

Limit error messages resulting from bad guest behavior to avoid allowing
the guest to cause the control domain's disk to fill.

The first message in pci_msix_write() can simply be deleted, as this
is indeed bad guest behavior, but such out of bounds writes don't
really need to be logged.

The second one is more problematic, as there guest behavior may only
appear to be wrong: For one, the old logic didn't take the mask-all bit
into account. And then this shouldn't depend on host device state (i.e.
the host may have masked the entry without the guest having done so).
Plus these writes shouldn't be dropped even when an entry is unmasked.
Instead, if they can't be made take effect right away, they should take
effect on the next unmasking or enabling operation - the specification
explicitly describes such caching behavior. Until we can validly drop
the message (implementing such caching/latching behavior), issue the
message just once per MSI-X table entry.

Note that the log message in pci_msix_read() similar to the one being
removed here is not an issue: "addr" being of unsigned type, and the
maximum size of the MSI-X table being 32k, entry_nr simply can't be
negative and hence the conditonal guarding issuing of the message will
never be true.

This is XSA-130.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt.h     |    1 +
 hw/xen/xen_pt_msi.c |   12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 52ceb85..8c9b6c2 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -175,6 +175,7 @@ typedef struct XenPTMSIXEntry {
     uint32_t data;
     uint32_t vector_ctrl;
     bool updated; /* indicate whether MSI ADDR or DATA is updated */
+    bool warned;  /* avoid issuing (bogus) warning more than once */
 } XenPTMSIXEntry;
 typedef struct XenPTMSIX {
     uint32_t ctrl_offset;
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 9ed9321..68db623 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -434,11 +434,10 @@ static void pci_msix_write(void *opaque, hwaddr addr,
     XenPCIPassthroughState *s = opaque;
     XenPTMSIX *msix = s->msix;
     XenPTMSIXEntry *entry;
-    int entry_nr, offset;
+    unsigned int entry_nr, offset;
 
     entry_nr = addr / PCI_MSIX_ENTRY_SIZE;
-    if (entry_nr < 0 || entry_nr >= msix->total_entries) {
-        XEN_PT_ERR(&s->dev, "asked MSI-X entry '%i' invalid!\n", entry_nr);
+    if (entry_nr >= msix->total_entries) {
         return;
     }
     entry = &msix->msix_entry[entry_nr];
@@ -460,8 +459,11 @@ static void pci_msix_write(void *opaque, hwaddr addr,
             + PCI_MSIX_ENTRY_VECTOR_CTRL;
 
         if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
-            XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
-                       " already enabled.\n", entry_nr);
+            if (!entry->warned) {
+                entry->warned = true;
+                XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
+                           " already enabled.\n", entry_nr);
+            }
             return;
         }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 04/11] xen/MSI: don't open-code pass-through of enable bit modifications

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

Without this the actual XSA-131 fix would cause the enable bit to not
get set anymore (due to the write back getting suppressed there based
on the OR of emu_mask, ro_mask, and res_mask).

Note that the fiddling with the enable bit shouldn't really be done by
qemu, but making this work right (via libxc and the hypervisor) will
require more extensive changes, which can be postponed until after the
security issue got addressed.

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt_config_init.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 68b8f22..436d0fd 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1053,7 +1053,6 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
     XenPTMSI *msi = s->msi;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = 0;
-    uint16_t raw_val;
 
     /* Currently no support for multi-vector */
     if (*val & PCI_MSI_FLAGS_QSIZE) {
@@ -1066,12 +1065,11 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
     msi->flags |= cfg_entry->data & ~PCI_MSI_FLAGS_ENABLE;
 
     /* create value for writing to I/O device register */
-    raw_val = *val;
     throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     /* update MSI */
-    if (raw_val & PCI_MSI_FLAGS_ENABLE) {
+    if (*val & PCI_MSI_FLAGS_ENABLE) {
         /* setup MSI pirq for the first time */
         if (!msi->initialized) {
             /* Init physical one */
@@ -1099,10 +1097,6 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
         xen_pt_msi_disable(s);
     }
 
-    /* pass through MSI_ENABLE bit */
-    *val &= ~PCI_MSI_FLAGS_ENABLE;
-    *val |= raw_val & PCI_MSI_FLAGS_ENABLE;
-
     return 0;
 }
 
@@ -1301,7 +1295,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .size       = 2,
         .init_val   = 0x0000,
         .ro_mask    = 0xFF8E,
-        .emu_mask   = 0x017F,
+        .emu_mask   = 0x017E,
         .init       = xen_pt_msgctrl_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_msgctrl_reg_write,
-- 
1.7.10.4

[Qemu-devel] [PATCH 05/11] xen/pt: consolidate PM capability emu_mask

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

There's no point in xen_pt_pmcsr_reg_{read,write}() each ORing
PCI_PM_CTRL_STATE_MASK and PCI_PM_CTRL_NO_SOFT_RESET into a local
emu_mask variable - we can have the same effect by setting the field
descriptor's emu_mask member suitably right away. Note that
xen_pt_pmcsr_reg_write() is being retained in order to allow later
patches to be less intrusive.

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 hw/xen/xen_pt_config_init.c |   25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 436d0fd..516236a 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -933,38 +933,21 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[] = {
  * Power Management Capability
  */
 
-/* read Power Management Control/Status register */
-static int xen_pt_pmcsr_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
-                                 uint16_t *value, uint16_t valid_mask)
-{
-    XenPTRegInfo *reg = cfg_entry->reg;
-    uint16_t valid_emu_mask = reg->emu_mask;
-
-    valid_emu_mask |= PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_NO_SOFT_RESET;
-
-    valid_emu_mask = valid_emu_mask & valid_mask;
-    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
-
-    return 0;
-}
 /* write Power Management Control/Status register */
 static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
                                   XenPTReg *cfg_entry, uint16_t *val,
                                   uint16_t dev_value, uint16_t valid_mask)
 {
     XenPTRegInfo *reg = cfg_entry->reg;
-    uint16_t emu_mask = reg->emu_mask;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = 0;
 
-    emu_mask |= PCI_PM_CTRL_STATE_MASK | PCI_PM_CTRL_NO_SOFT_RESET;
-
     /* modify emulate register */
-    writable_mask = emu_mask & ~reg->ro_mask & valid_mask;
+    writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~emu_mask & valid_mask;
+    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     return 0;
@@ -1000,9 +983,9 @@ static XenPTRegInfo xen_pt_emu_reg_pm[] = {
         .size       = 2,
         .init_val   = 0x0008,
         .ro_mask    = 0xE1FC,
-        .emu_mask   = 0x8100,
+        .emu_mask   = 0x810B,
         .init       = xen_pt_common_reg_init,
-        .u.w.read   = xen_pt_pmcsr_reg_read,
+        .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_pmcsr_reg_write,
     },
     {
-- 
1.7.10.4

[Qemu-devel] [PATCH 06/11] xen/pt: correctly handle PM status bit

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

xen_pt_pmcsr_reg_write() needs an adjustment to deal with the RW1C
nature of the not passed through bit 15 (PCI_PM_CTRL_PME_STATUS).

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt_config_init.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 516236a..027ac32 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -948,7 +948,8 @@ static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
 
     /* create value for writing to I/O device register */
     throughable_mask = ~reg->emu_mask & valid_mask;
-    *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+    *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
+                              throughable_mask);
 
     return 0;
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 07/11] xen/pt: split out calculation of throughable mask in PCI config space handling

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

This is just to avoid having to adjust that calculation later in
multiple places.

Note that including ->ro_mask in get_throughable_mask()'s calculation
is only an apparent (i.e. benign) behavioral change: For r/o fields it
doesn't matter > whether they get passed through - either the same flag
is also set in emu_mask (then there's no change at all) or the field is
r/o in hardware (and hence a write won't change it anyway).

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/xen/xen_pt_config_init.c |   51 +++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 027ac32..3833b9e 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -95,6 +95,14 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address)
     return NULL;
 }
 
+static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
+                                     const XenPTRegInfo *reg,
+                                     uint32_t valid_mask)
+{
+    uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
+
+    return throughable_mask & valid_mask;
+}
 
 /****************
  * general register functions
@@ -157,14 +165,13 @@ static int xen_pt_byte_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint8_t writable_mask = 0;
-    uint8_t throughable_mask = 0;
+    uint8_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     return 0;
@@ -175,14 +182,13 @@ static int xen_pt_word_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
+    uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     return 0;
@@ -193,14 +199,13 @@ static int xen_pt_long_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
-    uint32_t throughable_mask = 0;
+    uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     return 0;
@@ -292,15 +297,13 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
+    uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* modify emulate register */
     writable_mask = ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
-
     if (*val & PCI_COMMAND_INTX_DISABLE) {
         throughable_mask |= PCI_COMMAND_INTX_DISABLE;
     } else {
@@ -454,7 +457,6 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     PCIDevice *d = &s->dev;
     const PCIIORegion *r;
     uint32_t writable_mask = 0;
-    uint32_t throughable_mask = 0;
     uint32_t bar_emu_mask = 0;
     uint32_t bar_ro_mask = 0;
     uint32_t r_size = 0;
@@ -511,8 +513,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     }
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~bar_emu_mask & valid_mask;
-    *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+    *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     return 0;
 }
@@ -526,9 +527,8 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
     XenPTRegion *base = NULL;
     PCIDevice *d = (PCIDevice *)&s->dev;
     uint32_t writable_mask = 0;
-    uint32_t throughable_mask = 0;
+    uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
     pcibus_t r_size = 0;
-    uint32_t bar_emu_mask = 0;
     uint32_t bar_ro_mask = 0;
 
     r_size = d->io_regions[PCI_ROM_SLOT].size;
@@ -537,7 +537,6 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
     r_size = xen_pt_get_emul_size(base->bar_flag, r_size);
 
     /* set emulate mask and read-only mask */
-    bar_emu_mask = reg->emu_mask;
     bar_ro_mask = (reg->ro_mask | (r_size - 1)) & ~PCI_ROM_ADDRESS_ENABLE;
 
     /* modify emulate register */
@@ -545,7 +544,6 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~bar_emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     return 0;
@@ -940,14 +938,13 @@ static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
+    uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
                               throughable_mask);
 
@@ -1036,7 +1033,7 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
     XenPTRegInfo *reg = cfg_entry->reg;
     XenPTMSI *msi = s->msi;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
+    uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
 
     /* Currently no support for multi-vector */
     if (*val & PCI_MSI_FLAGS_QSIZE) {
@@ -1049,7 +1046,6 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
     msi->flags |= cfg_entry->data & ~PCI_MSI_FLAGS_ENABLE;
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     /* update MSI */
@@ -1161,7 +1157,6 @@ static int xen_pt_msgaddr32_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
-    uint32_t throughable_mask = 0;
     uint32_t old_addr = cfg_entry->data;
 
     /* modify emulate register */
@@ -1170,8 +1165,7 @@ static int xen_pt_msgaddr32_reg_write(XenPCIPassthroughState *s,
     s->msi->addr_lo = cfg_entry->data;
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
-    *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+    *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
     if (cfg_entry->data != old_addr) {
@@ -1189,7 +1183,6 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
-    uint32_t throughable_mask = 0;
     uint32_t old_addr = cfg_entry->data;
 
     /* check whether the type is 64 bit or not */
@@ -1206,8 +1199,7 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
     s->msi->addr_hi = cfg_entry->data;
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
-    *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+    *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
     if (cfg_entry->data != old_addr) {
@@ -1229,7 +1221,6 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     XenPTRegInfo *reg = cfg_entry->reg;
     XenPTMSI *msi = s->msi;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
     uint16_t old_data = cfg_entry->data;
     uint32_t offset = reg->offset;
 
@@ -1247,8 +1238,7 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     msi->data = cfg_entry->data;
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
-    *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+    *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
     if (cfg_entry->data != old_data) {
@@ -1410,7 +1400,7 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
-    uint16_t throughable_mask = 0;
+    uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
     int debug_msix_enabled_old;
 
     /* modify emulate register */
@@ -1418,7 +1408,6 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
     cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
 
     /* create value for writing to I/O device register */
-    throughable_mask = ~reg->emu_mask & valid_mask;
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
 
     /* update MSI-X */
-- 
1.7.10.4

[Qemu-devel] [PATCH 08/11] xen/pt: mark all PCIe capability bits read-only

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

xen_pt_emu_reg_pcie[]'s PCI_EXP_DEVCAP needs to cover all bits as read-
only to avoid unintended write-back (just a precaution, the field ought
to be read-only in hardware).

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt_config_init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3833b9e..9f6c00e 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -871,7 +871,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[] = {
         .offset     = PCI_EXP_DEVCAP,
         .size       = 4,
         .init_val   = 0x00000000,
-        .ro_mask    = 0x1FFCFFFF,
+        .ro_mask    = 0xFFFFFFFF,
         .emu_mask   = 0x10000000,
         .init       = xen_pt_common_reg_init,
         .u.dw.read  = xen_pt_long_reg_read,
-- 
1.7.10.4

[Qemu-devel] [PATCH 09/11] xen/pt: mark reserved bits in PCI config space fields

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

The adjustments are solely to make the subsequent patches work right
(and hence make the patch set consistent), namely if permissive mode
(introduced by the last patch) gets used (as both reserved registers
and reserved fields must be similarly protected from guest access in
default mode, but the guest should be allowed access to them in
permissive mode).

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 hw/xen/xen_pt.h             |    2 ++
 hw/xen/xen_pt_config_init.c |   14 +++++++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 8c9b6c2..f9795eb 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -101,6 +101,8 @@ struct XenPTRegInfo {
     uint32_t offset;
     uint32_t size;
     uint32_t init_val;
+    /* reg reserved field mask (ON:reserved, OFF:defined) */
+    uint32_t res_mask;
     /* reg read only field mask (ON:RO/ROS, OFF:other) */
     uint32_t ro_mask;
     /* reg emulate field mask (ON:emu, OFF:passthrough) */
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 9f6c00e..efd8bac 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -578,7 +578,7 @@ static XenPTRegInfo xen_pt_emu_reg_header0[] = {
         .offset     = PCI_COMMAND,
         .size       = 2,
         .init_val   = 0x0000,
-        .ro_mask    = 0xF880,
+        .res_mask   = 0xF880,
         .emu_mask   = 0x0743,
         .init       = xen_pt_common_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
@@ -603,7 +603,8 @@ static XenPTRegInfo xen_pt_emu_reg_header0[] = {
         .offset     = PCI_STATUS,
         .size       = 2,
         .init_val   = 0x0000,
-        .ro_mask    = 0x06FF,
+        .res_mask   = 0x0007,
+        .ro_mask    = 0x06F8,
         .emu_mask   = 0x0010,
         .init       = xen_pt_status_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
@@ -980,7 +981,8 @@ static XenPTRegInfo xen_pt_emu_reg_pm[] = {
         .offset     = PCI_PM_CTRL,
         .size       = 2,
         .init_val   = 0x0008,
-        .ro_mask    = 0xE1FC,
+        .res_mask   = 0x00F0,
+        .ro_mask    = 0xE10C,
         .emu_mask   = 0x810B,
         .init       = xen_pt_common_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
@@ -1268,7 +1270,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] = {
         .offset     = PCI_MSI_FLAGS,
         .size       = 2,
         .init_val   = 0x0000,
-        .ro_mask    = 0xFF8E,
+        .res_mask   = 0xFE00,
+        .ro_mask    = 0x018E,
         .emu_mask   = 0x017E,
         .init       = xen_pt_msgctrl_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
@@ -1446,7 +1449,8 @@ static XenPTRegInfo xen_pt_emu_reg_msix[] = {
         .offset     = PCI_MSI_FLAGS,
         .size       = 2,
         .init_val   = 0x0000,
-        .ro_mask    = 0x3FFF,
+        .res_mask   = 0x3800,
+        .ro_mask    = 0x07FF,
         .emu_mask   = 0x0000,
         .init       = xen_pt_msixctrl_reg_init,
         .u.w.read   = xen_pt_word_reg_read,
-- 
1.7.10.4

[Qemu-devel] [PATCH 10/11] xen/pt: add a few PCI config space field descriptions

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

Since the next patch will turn all not explicitly described fields
read-only by default, those fields that have guest writable bits need
to be given explicit descriptors.

This is a preparatory patch for XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 hw/xen/xen_pt_config_init.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index efd8bac..19f926b 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -754,6 +754,15 @@ static XenPTRegInfo xen_pt_emu_reg_vpd[] = {
         .u.b.write  = xen_pt_byte_reg_write,
     },
     {
+        .offset     = PCI_VPD_ADDR,
+        .size       = 2,
+        .ro_mask    = 0x0003,
+        .emu_mask   = 0x0003,
+        .init       = xen_pt_common_reg_init,
+        .u.w.read   = xen_pt_word_reg_read,
+        .u.w.write  = xen_pt_word_reg_write,
+    },
+    {
         .size = 0,
     },
 };
@@ -889,6 +898,16 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[] = {
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_word_reg_write,
     },
+    /* Device Status reg */
+    {
+        .offset     = PCI_EXP_DEVSTA,
+        .size       = 2,
+        .res_mask   = 0xFFC0,
+        .ro_mask    = 0x0030,
+        .init       = xen_pt_common_reg_init,
+        .u.w.read   = xen_pt_word_reg_read,
+        .u.w.write  = xen_pt_word_reg_write,
+    },
     /* Link Control reg */
     {
         .offset     = PCI_EXP_LNKCTL,
@@ -900,6 +919,15 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[] = {
         .u.w.read   = xen_pt_word_reg_read,
         .u.w.write  = xen_pt_word_reg_write,
     },
+    /* Link Status reg */
+    {
+        .offset     = PCI_EXP_LNKSTA,
+        .size       = 2,
+        .ro_mask    = 0x3FFF,
+        .init       = xen_pt_common_reg_init,
+        .u.w.read   = xen_pt_word_reg_read,
+        .u.w.write  = xen_pt_word_reg_write,
+    },
     /* Device Control 2 reg */
     {
         .offset     = 0x28,
-- 
1.7.10.4

[Qemu-devel] [PATCH 11/11] xen/pt: unknown PCI config space fields should be read-only

[Stefano Stabellini]

From: Jan Beulich <jbeulich@suse.com>

... by default. Add a per-device "permissive" mode similar to pciback's
to allow restoring previous behavior (and hence break security again,
i.e. should be used only for trusted guests).

This is part of XSA-131.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>)
---
 hw/xen/xen_pt.c             |   32 +++++++++++++++++++++++++++++---
 hw/xen/xen_pt.h             |    2 ++
 hw/xen/xen_pt_config_init.c |    4 ++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 8923582..9afcda8 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -239,6 +239,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     XenPTReg *reg_entry = NULL;
     uint32_t find_addr = addr;
     XenPTRegInfo *reg = NULL;
+    bool wp_flag = false;
 
     if (xen_pt_pci_config_access_check(d, addr, len)) {
         return;
@@ -278,6 +279,10 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
 
     /* pass directly to the real device for passthrough type register group */
     if (reg_grp_entry == NULL) {
+        if (!s->permissive) {
+            wb_mask = 0;
+            wp_flag = true;
+        }
         goto out;
     }
 
@@ -298,12 +303,15 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
             uint32_t real_offset = reg_grp_entry->base_offset + reg->offset;
             uint32_t valid_mask = 0xFFFFFFFF >> ((4 - emul_len) << 3);
             uint8_t *ptr_val = NULL;
+            uint32_t wp_mask = reg->emu_mask | reg->ro_mask;
 
             valid_mask <<= (find_addr - real_offset) << 3;
             ptr_val = (uint8_t *)&val + (real_offset & 3);
-            if (reg->emu_mask == (0xFFFFFFFF >> ((4 - reg->size) << 3))) {
-                wb_mask &= ~((reg->emu_mask
-                              >> ((find_addr - real_offset) << 3))
+            if (!s->permissive) {
+                wp_mask |= reg->res_mask;
+            }
+            if (wp_mask == (0xFFFFFFFF >> ((4 - reg->size) << 3))) {
+                wb_mask &= ~((wp_mask >> ((find_addr - real_offset) << 3))
                              << ((len - emul_len) << 3));
             }
 
@@ -347,6 +355,16 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
         } else {
             /* nothing to do with passthrough type register,
              * continue to find next byte */
+            if (!s->permissive) {
+                wb_mask &= ~(0xff << ((len - emul_len) << 3));
+                /* Unused BARs will make it here, but we don't want to issue
+                 * warnings for writes to them (bogus writes get dealt with
+                 * above).
+                 */
+                if (index < 0) {
+                    wp_flag = true;
+                }
+            }
             emul_len--;
             find_addr++;
         }
@@ -358,6 +376,13 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
     memory_region_transaction_commit();
 
 out:
+    if (wp_flag && !s->permissive_warned) {
+        s->permissive_warned = true;
+        xen_pt_log(d, "Write-back to unknown field 0x%02x (partially) inhibited (0x%0*x)\n",
+                   addr, len * 2, wb_mask);
+        xen_pt_log(d, "If the device doesn't work, try enabling permissive mode\n");
+        xen_pt_log(d, "(unsafe) and if it helps report the problem to xen-devel\n");
+    }
     for (index = 0; wb_mask; index += len) {
         /* unknown regs are passed through */
         while (!(wb_mask & 0xff)) {
@@ -824,6 +849,7 @@ static void xen_pt_unregister_device(PCIDevice *d)
 
 static Property xen_pci_passthrough_properties[] = {
     DEFINE_PROP_PCI_HOST_DEVADDR("hostaddr", XenPCIPassthroughState, hostaddr),
+    DEFINE_PROP_BOOL("permissive", XenPCIPassthroughState, permissive, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index f9795eb..4bba559 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -197,6 +197,8 @@ struct XenPCIPassthroughState {
 
     PCIHostDeviceAddress hostaddr;
     bool is_virtfn;
+    bool permissive;
+    bool permissive_warned;
     XenHostPCIDevice real_device;
     XenPTRegion bases[PCI_NUM_REGIONS]; /* Access regions */
     QLIST_HEAD(, XenPTRegGroup) reg_grps;
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 19f926b..f3cf069 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -101,6 +101,10 @@ static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
 {
     uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
 
+    if (!s->permissive) {
+        throughable_mask &= ~reg->res_mask;
+    }
+
     return throughable_mask & valid_mask;
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Stefano Stabellini]

On Tue, 2 Jun 2015, Stefano Stabellini wrote:
> Hi all,
> 
> the following is a collection of QEMU security fixes for PCI Passthrough
> on Xen. Non-Xen usages of QEMU are unaffected.
> 
> Although the CVEs have already been made public, given the large amount
> of changes, I decided not to send a pull request without giving a chance
> to the QEMU community to comment on the patches first.

Peter convinced me to send out a pull request immediately. If anybody
has any comments on the patches, we can still fix them up later or even
revert them if that becomes necessary.

I'll also apply the patches to all qemu-xen stable trees now.


> Each patch has a detail description of what is trying to fix. You can
> also cross-reference the CVE numbers.
> 
> 
> 
> Jan Beulich (11):
>       xen: properly gate host writes of modified PCI CFG contents
>       xen: don't allow guest to control MSI mask register
>       xen/MSI-X: limit error messages
>       xen/MSI: don't open-code pass-through of enable bit modifications
>       xen/pt: consolidate PM capability emu_mask
>       xen/pt: correctly handle PM status bit
>       xen/pt: split out calculation of throughable mask in PCI config space handling
>       xen/pt: mark all PCIe capability bits read-only
>       xen/pt: mark reserved bits in PCI config space fields
>       xen/pt: add a few PCI config space field descriptions
>       xen/pt: unknown PCI config space fields should be read-only
> 
>  hw/pci/msi.c                |    4 -
>  hw/xen/xen_pt.c             |   51 +++++++++-
>  hw/xen/xen_pt.h             |    7 +-
>  hw/xen/xen_pt_config_init.c |  235 ++++++++++++++++++++++++++++---------------
>  hw/xen/xen_pt_msi.c         |   12 ++-
>  include/hw/pci/pci_regs.h   |    2 +
>  6 files changed, 217 insertions(+), 94 deletions(-)
> 
> 
> Cheers,
> 
> Stefano
> 

Re: [Qemu-devel] [Xen-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Ian Campbell]

On Tue, 2015-06-02 at 16:08 +0100, Stefano Stabellini wrote:
> the following is a collection of QEMU security fixes for PCI Passthrough
> on Xen.

Part of this locks down the PCI cfg space emulation, which means we now
need a way for people to request the old "permissive" behaviour for
devices which need it. Per the xl docs:
        It is recommended to enable this option only for trusted VMs
        under administrator control.

The toolstack (libxl, xl etc) already support a permissive flag in the
domain cfg, and this series adds a new device property. All we need to
do is tie them together.

The simple version is below. I also have an incremental update which
uses the QMP device-list-properties command to probe for the presence of
this property (so things can automatically work with unpatches qemu). I
think it's not really necessary in this case.

Ian.

-----8>---------

>From c395657b03a1e2b7616d987e7078694874981979 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Mon, 1 Jun 2015 11:32:23 +0100
Subject: [PATCH] tools: libxl: allow permissive qemu-upstream pci
 passthrough.

EMBARGOED UNTIL 2015-06-02 12:00 (WITH XSA-131 ET AL)

Since XSA-131 qemu-xen now restricts access to PCI cfg by default. In
order to allow local configuration of the existing libxl_device_pci
"permissive" flag needs to be plumbed through via the new QMP property
added by the XSA-131 patches.

Versions of QEMU prior to XSA-131 did not support this permissive
property, so we only pass it if it is true. Older versions only
supported permissive mode.

qemu-xen-traditional already supports the permissive mode setting via
xenstore.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Only set argument if permissive==true.
---
 tools/libxl/libxl_qmp.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9aa7e2e..6484f5e 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -849,6 +849,18 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
         QMP_PARAMETERS_SPRINTF(&args, "addr", "%x.%x",
                                PCI_SLOT(pcidev->vdevfn), PCI_FUNC(pcidev->vdevfn));
     }
+    /*
+     * Version of QEMU prior to the XSA-131 fix did not support this
+     * property and were effectively always in permissive mode. The
+     * fix for XSA-131 switched the default to be restricted by
+     * default and added the permissive property.
+     *
+     * Therefore in order to support both old and new QEMU we only set
+     * the permissive flag if it is true. Users of older QEMU have no
+     * reason to set the flag so this is ok.
+     */
+    if (pcidev->permissive)
+        qmp_parameters_add_bool(gc, &args, "permissive", true);
 
     rc = qmp_synchronous_send(qmp, "device_add", args,
                               NULL, NULL, qmp->timeout);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Peter Maydell]

On 2 June 2015 at 16:32, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 2 Jun 2015, Stefano Stabellini wrote:
>> Hi all,
>>
>> the following is a collection of QEMU security fixes for PCI Passthrough
>> on Xen. Non-Xen usages of QEMU are unaffected.
>>
>> Although the CVEs have already been made public, given the large amount
>> of changes, I decided not to send a pull request without giving a chance
>> to the QEMU community to comment on the patches first.
>
> Peter convinced me to send out a pull request immediately. If anybody
> has any comments on the patches, we can still fix them up later or even
> revert them if that becomes necessary.

For the record, the rationale is:
 * fixes for CVEs will have been reviewed during the nondisclosure period
 * getting security fixes into master in a timely fashion is important
 * having the patches in upstream master be different from the ones
   advertised with the CVE can cause confusion about which are "correct"
 * if there are any problems with the CVE fixes (stylistic or otherwise)
   we can correct them with followup patches

Our workflow/process for handling security issues is not set in
stone (indeed it's evolving a bit at the moment), so comments/suggestions
welcome.

thanks
-- PMM

Re: [Qemu-devel] [Xen-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Ian Campbell]

On Tue, 2015-06-02 at 16:47 +0100, Ian Campbell wrote:

ping?

> On Tue, 2015-06-02 at 16:08 +0100, Stefano Stabellini wrote:
> > the following is a collection of QEMU security fixes for PCI Passthrough
> > on Xen.
> 
> Part of this locks down the PCI cfg space emulation, which means we now
> need a way for people to request the old "permissive" behaviour for
> devices which need it. Per the xl docs:
>         It is recommended to enable this option only for trusted VMs
>         under administrator control.
> 
> The toolstack (libxl, xl etc) already support a permissive flag in the
> domain cfg, and this series adds a new device property. All we need to
> do is tie them together.
> 
> The simple version is below. I also have an incremental update which
> uses the QMP device-list-properties command to probe for the presence of
> this property (so things can automatically work with unpatches qemu). I
> think it's not really necessary in this case.
> 
> Ian.
> 
> -----8>---------
> 
> From c395657b03a1e2b7616d987e7078694874981979 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Mon, 1 Jun 2015 11:32:23 +0100
> Subject: [PATCH] tools: libxl: allow permissive qemu-upstream pci
>  passthrough.
> 
> EMBARGOED UNTIL 2015-06-02 12:00 (WITH XSA-131 ET AL)
> 
> Since XSA-131 qemu-xen now restricts access to PCI cfg by default. In
> order to allow local configuration of the existing libxl_device_pci
> "permissive" flag needs to be plumbed through via the new QMP property
> added by the XSA-131 patches.
> 
> Versions of QEMU prior to XSA-131 did not support this permissive
> property, so we only pass it if it is true. Older versions only
> supported permissive mode.
> 
> qemu-xen-traditional already supports the permissive mode setting via
> xenstore.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v2: Only set argument if permissive==true.
> ---
>  tools/libxl/libxl_qmp.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 9aa7e2e..6484f5e 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -849,6 +849,18 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
>          QMP_PARAMETERS_SPRINTF(&args, "addr", "%x.%x",
>                                 PCI_SLOT(pcidev->vdevfn), PCI_FUNC(pcidev->vdevfn));
>      }
> +    /*
> +     * Version of QEMU prior to the XSA-131 fix did not support this
> +     * property and were effectively always in permissive mode. The
> +     * fix for XSA-131 switched the default to be restricted by
> +     * default and added the permissive property.
> +     *
> +     * Therefore in order to support both old and new QEMU we only set
> +     * the permissive flag if it is true. Users of older QEMU have no
> +     * reason to set the flag so this is ok.
> +     */
> +    if (pcidev->permissive)
> +        qmp_parameters_add_bool(gc, &args, "permissive", true);
>  
>      rc = qmp_synchronous_send(qmp, "device_add", args,
>                                NULL, NULL, qmp->timeout);

Re: [Qemu-devel] [Xen-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Stefano Stabellini]

On Wed, 17 Jun 2015, Ian Campbell wrote:
> On Tue, 2015-06-02 at 16:47 +0100, Ian Campbell wrote:
> 
> ping?

The QEMU patch series is already upstream in QEMU and qemu-xen, so I
think we can apply this patch to libxl now.


> > On Tue, 2015-06-02 at 16:08 +0100, Stefano Stabellini wrote:
> > > the following is a collection of QEMU security fixes for PCI Passthrough
> > > on Xen.
> > 
> > Part of this locks down the PCI cfg space emulation, which means we now
> > need a way for people to request the old "permissive" behaviour for
> > devices which need it. Per the xl docs:
> >         It is recommended to enable this option only for trusted VMs
> >         under administrator control.
> > 
> > The toolstack (libxl, xl etc) already support a permissive flag in the
> > domain cfg, and this series adds a new device property. All we need to
> > do is tie them together.
> > 
> > The simple version is below. I also have an incremental update which
> > uses the QMP device-list-properties command to probe for the presence of
> > this property (so things can automatically work with unpatches qemu). I
> > think it's not really necessary in this case.
> > 
> > Ian.
> > 
> > -----8>---------
> > 
> > From c395657b03a1e2b7616d987e7078694874981979 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Mon, 1 Jun 2015 11:32:23 +0100
> > Subject: [PATCH] tools: libxl: allow permissive qemu-upstream pci
> >  passthrough.
> > 
> > EMBARGOED UNTIL 2015-06-02 12:00 (WITH XSA-131 ET AL)
> > 
> > Since XSA-131 qemu-xen now restricts access to PCI cfg by default. In
> > order to allow local configuration of the existing libxl_device_pci
> > "permissive" flag needs to be plumbed through via the new QMP property
> > added by the XSA-131 patches.
> > 
> > Versions of QEMU prior to XSA-131 did not support this permissive
> > property, so we only pass it if it is true. Older versions only
> > supported permissive mode.
> > 
> > qemu-xen-traditional already supports the permissive mode setting via
> > xenstore.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > v2: Only set argument if permissive==true.
> > ---
> >  tools/libxl/libxl_qmp.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index 9aa7e2e..6484f5e 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > @@ -849,6 +849,18 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
> >          QMP_PARAMETERS_SPRINTF(&args, "addr", "%x.%x",
> >                                 PCI_SLOT(pcidev->vdevfn), PCI_FUNC(pcidev->vdevfn));
> >      }
> > +    /*
> > +     * Version of QEMU prior to the XSA-131 fix did not support this
> > +     * property and were effectively always in permissive mode. The
> > +     * fix for XSA-131 switched the default to be restricted by
> > +     * default and added the permissive property.
> > +     *
> > +     * Therefore in order to support both old and new QEMU we only set
> > +     * the permissive flag if it is true. Users of older QEMU have no
> > +     * reason to set the flag so this is ok.
> > +     */
> > +    if (pcidev->permissive)
> > +        qmp_parameters_add_bool(gc, &args, "permissive", true);
> >  
> >      rc = qmp_synchronous_send(qmp, "device_add", args,
> >                                NULL, NULL, qmp->timeout);
> 
> 

Re: [Qemu-devel] [Xen-devel] [PATCH 0/11] Xen PCI Passthrough security fixes

[Ian Campbell]

On Wed, 2015-06-17 at 14:52 +0100, Stefano Stabellini wrote:
> On Wed, 17 Jun 2015, Ian Campbell wrote:
> > On Tue, 2015-06-02 at 16:47 +0100, Ian Campbell wrote:
> > 
> > ping?
> 
> The QEMU patch series is already upstream in QEMU and qemu-xen, so I
> think we can apply this patch to libxl now.

Yes, which is why somebody should Ack or Reviewed-by it.

> 
> > > On Tue, 2015-06-02 at 16:08 +0100, Stefano Stabellini wrote:
> > > > the following is a collection of QEMU security fixes for PCI Passthrough
> > > > on Xen.
> > > 
> > > Part of this locks down the PCI cfg space emulation, which means we now
> > > need a way for people to request the old "permissive" behaviour for
> > > devices which need it. Per the xl docs:
> > >         It is recommended to enable this option only for trusted VMs
> > >         under administrator control.
> > > 
> > > The toolstack (libxl, xl etc) already support a permissive flag in the
> > > domain cfg, and this series adds a new device property. All we need to
> > > do is tie them together.
> > > 
> > > The simple version is below. I also have an incremental update which
> > > uses the QMP device-list-properties command to probe for the presence of
> > > this property (so things can automatically work with unpatches qemu). I
> > > think it's not really necessary in this case.
> > > 
> > > Ian.
> > > 
> > > -----8>---------
> > > 
> > > From c395657b03a1e2b7616d987e7078694874981979 Mon Sep 17 00:00:00 2001
> > > From: Ian Campbell <ian.campbell@citrix.com>
> > > Date: Mon, 1 Jun 2015 11:32:23 +0100
> > > Subject: [PATCH] tools: libxl: allow permissive qemu-upstream pci
> > >  passthrough.
> > > 
> > > EMBARGOED UNTIL 2015-06-02 12:00 (WITH XSA-131 ET AL)
> > > 
> > > Since XSA-131 qemu-xen now restricts access to PCI cfg by default. In
> > > order to allow local configuration of the existing libxl_device_pci
> > > "permissive" flag needs to be plumbed through via the new QMP property
> > > added by the XSA-131 patches.
> > > 
> > > Versions of QEMU prior to XSA-131 did not support this permissive
> > > property, so we only pass it if it is true. Older versions only
> > > supported permissive mode.
> > > 
> > > qemu-xen-traditional already supports the permissive mode setting via
> > > xenstore.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > > v2: Only set argument if permissive==true.
> > > ---
> > >  tools/libxl/libxl_qmp.c |   12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > > index 9aa7e2e..6484f5e 100644
> > > --- a/tools/libxl/libxl_qmp.c
> > > +++ b/tools/libxl/libxl_qmp.c
> > > @@ -849,6 +849,18 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
> > >          QMP_PARAMETERS_SPRINTF(&args, "addr", "%x.%x",
> > >                                 PCI_SLOT(pcidev->vdevfn), PCI_FUNC(pcidev->vdevfn));
> > >      }
> > > +    /*
> > > +     * Version of QEMU prior to the XSA-131 fix did not support this
> > > +     * property and were effectively always in permissive mode. The
> > > +     * fix for XSA-131 switched the default to be restricted by
> > > +     * default and added the permissive property.
> > > +     *
> > > +     * Therefore in order to support both old and new QEMU we only set
> > > +     * the permissive flag if it is true. Users of older QEMU have no
> > > +     * reason to set the flag so this is ok.
> > > +     */
> > > +    if (pcidev->permissive)
> > > +        qmp_parameters_add_bool(gc, &args, "permissive", true);
> > >  
> > >      rc = qmp_synchronous_send(qmp, "device_add", args,
> > >                                NULL, NULL, qmp->timeout);
> > 
> >