[Qemu-devel] [PATCH] pci: Factor out bounds checking on config space accesses

[David Gibson]

There are several paths into the code to emulate PCI config space accesses:
one for MMIO to a plain old PCI bridge one for MMIO to a PCIe bridge and
one for the pseries machine which provides para-virtualized access to PCI
config space.  Each of these functions does their own bounds checking
against the size of config space to check for addresses outside the
size of config space.  The pci_host_config_{read,write}_common() (sort
of) checks for partial overruns, that is where the address is within
the size of config space, but address + length is not, it takes a
limit parameter for this purpose.

As well as being a small code duplication, and it being weird to
separate the checks for partial and total overruns, this checking
currently has a few buglets:

    * For non PCI-Express we assume that the size of config space is
      PCI_CONFIG_SPACE_SIZE.  That's true for everything we emulate
      now, but is not necessarily true (e.g. PCI-X devices can have
      extended config space)

    * The limit parameter is not necessary, since the size of config
      space can be obtained using pci_config_size()

    * Partial overruns could only occur with a misaligned access,
      which should have already been dealt with by this point

    * Partial overruns are handled as a partial read or write, which
      is very unlikely behaviour for real hardware

    * Furthermore, partial reads are 0x0 padded, whereas returning
      0xff for unimplemented addresses us much more common.

    * The partial reads/writes only work correctly by assuming
      little-endian byte layout.  While that is always true for PCI
      config space, it's an awfully subtle thing to rely on without
      comment.

This patch, therefore, moves the bounds checking wholly into
pci_host_config_{read,write}_common().  No partial reads or writes are
performed, instead any out-of-bounds write is simply ignored and an
out-of-bounds read returns 0xff.

This simplifies all the callers, and makes the overall semantics saner
for edge cases.

Cc: Michael S. Tsirkin <address@hidden>

Signed-off-by: David Gibson <address@hidden>
---
 hw/pci_host.c  |   26 ++++++++++++++------------
 hw/pci_host.h  |    4 ++--
 hw/pcie_host.c |   18 ++----------------
 hw/spapr_pci.c |   27 ++++-----------------------
 4 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..829d797 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -48,48 +48,50 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, 
uint32_t addr)
 }
 
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len)
+                                  uint32_t val, uint32_t len)
 {
     assert(len <= 4);
-    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        pci_dev->config_write(pci_dev, addr, val, len);
+    }
 }
 
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len)
+                                     uint32_t len)
 {
     assert(len <= 4);
-    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        return pci_dev->config_read(pci_dev, addr, len);
+    } else {
+        return ~0x0;
+    }
 }
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev) {
         return;
     }
 
     PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
-    pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
-                                 val, len);
+                __func__, pci_dev->name, addr, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
     if (!pci_dev) {
         return ~0x0;
     }
 
-    val = pci_host_config_read_common(pci_dev, config_addr,
-                                      PCI_CONFIG_SPACE_SIZE, len);
+    val = pci_host_config_read_common(pci_dev, addr, len);
     PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
+                __func__, pci_dev->name, addr, val, len);
 
     return val;
 }
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 359e38f..4bb0838 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -42,9 +42,9 @@ struct PCIHostState {
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len);
+                                  uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len);
+                                     uint32_t len);
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index 28bbe72..3af8610 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -60,19 +60,12 @@ static void pcie_mmcfg_data_write(void *opaque, 
target_phys_addr_t mmcfg_addr,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return;
-    }
-    pci_host_config_write_common(pci_dev, addr, limit, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 static uint64_t pcie_mmcfg_data_read(void *opaque,
@@ -83,19 +76,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return ~0x0;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return ~0x0;
-    }
-    return pci_host_config_read_common(pci_dev, addr, limit, len);
+    return pci_host_config_read_common(pci_dev, addr, len);
 }
 
 static const MemoryRegionOps pcie_mmcfg_ops = {
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index e7ef551..77f3e52 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -60,25 +60,6 @@ static uint32_t rtas_pci_cfgaddr(uint32_t arg)
     return ((arg >> 20) & 0xf00) | (arg & 0xff);
 }
 
-static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                        uint32_t limit, uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        return pci_host_config_read_common(pci_dev, addr, limit, len);
-    } else {
-        return ~0x0;
-    }
-}
-
-static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t val,
-                                     uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        pci_host_config_write_common(pci_dev, addr, limit, val, len);
-    }
-}
-
 static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
                                      uint32_t token, uint32_t nargs,
                                      target_ulong args,
@@ -94,7 +75,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -113,7 +94,7 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -134,7 +115,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment 
*spapr,
     val = rtas_ld(args, 4);
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
@@ -153,7 +134,7 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
     val = rtas_ld(args, 2);
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
-- 
1.7.9.1