[Qemu-devel] [PATCH] pci: Remove partial overrun checking from pci_host_config_{read, write} common

[David Gibson]

Currently the pci_host_config_{read,write}_common() functions clamp the
given access size to prevent it from overruning the size of config space.
This does not protect against "total" overruns (that is where the start
address is outside config space), but given some correct but rather subtle
assumptions does handle partial overruns (addr is within config space, but
the access size overruns it) as a truncated read or write.

A truncated read or write is vanishingly unlikely to be performed by real
hardware, but more importantly, this code path will never be executed. The
callers of pci_host_config_{read,write}_common() already check that the
access is not a total overrun and is naturally aligned.  Together those
mean that a partial overrun is not possible either.

This patch, therefore, removes the size clamping and the associated 'limit'
parameter to pci_host_config_{read,write}_common().  We do add an
assert instead, which will catch cases where the caller doesn't
properly handle overruns (either total or partial).

Cc: Michael S. Tsirkin <address@hidden>

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

Michael, latest take on my attempt to clean up the bounds checking on
config space access.  I'm hoping with the actual bug fix for pseries
already applied, the innocuousness of this part of the cleanup will
now be apparent.

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..0c298dd 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -48,17 +48,17 @@ 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));
+    assert((len <= 4) && ((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));
+    assert((len <= 4) && ((addr + len) <= pci_config_size(pci_dev)));
+    return pci_dev->config_read(pci_dev, addr, len);
 }
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
@@ -72,8 +72,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, 
int len)
 
     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);
+    pci_host_config_write_common(pci_dev, config_addr, val, len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
@@ -86,8 +85,7 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
         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, config_addr, len);
     PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
                 __func__, pci_dev->name, config_addr, val, len);
 
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..1bdca34 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -72,7 +72,7 @@ static void pcie_mmcfg_data_write(void *opaque, 
target_phys_addr_t mmcfg_addr,
            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,
@@ -95,7 +95,7 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
            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 a564c00..72309d9 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -84,8 +84,7 @@ static void finish_read_pci_config(sPAPREnvironment *spapr, 
uint64_t buid,
         return;
     }
 
-    val = pci_host_config_read_common(pci_dev, addr,
-                                      pci_config_size(pci_dev), size);
+    val = pci_host_config_read_common(pci_dev, addr, size);
 
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
@@ -151,9 +150,8 @@ static void finish_write_pci_config(sPAPREnvironment 
*spapr, uint64_t buid,
         return;
     }
 
-    pci_host_config_write_common(pci_dev, addr, pci_config_size(pci_dev),
-                                 val, size);
-
+    pci_host_config_write_common(pci_dev, addr, val, size);
+    
     rtas_st(rets, 0, 0);
 }
 
-- 
1.7.9.5