Patchwork pci: Common overflow prevention

login
register
mail settings
Submitter Jan Kiszka
Date July 21, 2011, 4:50 p.m.
Message ID <4E2858C2.5050909@siemens.com>
Download mbox | patch
Permalink /patch/106108/
State New
Headers show

Comments

Jan Kiszka - July 21, 2011, 4:50 p.m.
Introduce pci_config_read/write_common helpers to prevent passing
accesses down the callback chain that go beyond the config space limits.
Adjust length assertions as they are no longer correct (cutting may
generate valid 3 byte accesses).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Now I have to deal with 3 byte config space access for device
assignment, but Michael was right, such things are possible, even in
PCIe.

 hw/pci.c       |    6 ++----
 hw/pci_host.c  |   24 ++++++++++++++++++++----
 hw/pci_host.h  |    6 ++++++
 hw/pcie_host.c |   12 ++++++------
 4 files changed, 34 insertions(+), 14 deletions(-)
Michael S. Tsirkin - July 22, 2011, 5:32 a.m.
On Thu, Jul 21, 2011 at 06:50:10PM +0200, Jan Kiszka wrote:
> Introduce pci_config_read/write_common helpers to prevent passing
> accesses down the callback chain that go beyond the config space limits.
> Adjust length assertions as they are no longer correct (cutting may
> generate valid 3 byte accesses).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Now I have to deal with 3 byte config space access for device
> assignment, but Michael was right, such things are possible, even in
> PCIe.
> 
>  hw/pci.c       |    6 ++----
>  hw/pci_host.c  |   24 ++++++++++++++++++++----
>  hw/pci_host.h  |    6 ++++++
>  hw/pcie_host.c |   12 ++++++------
>  4 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index b904a4e..ef94739 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1108,8 +1108,7 @@ uint32_t pci_default_read_config(PCIDevice *d,
>                                   uint32_t address, int len)
>  {
>      uint32_t val = 0;
> -    assert(len == 1 || len == 2 || len == 4);
> -    len = MIN(len, pci_config_size(d) - address);
> +
>      memcpy(&val, d->config + address, len);
>      return le32_to_cpu(val);
>  }
> @@ -1117,9 +1116,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
>      int i, was_irq_disabled = pci_irq_disabled(d);
> -    uint32_t config_size = pci_config_size(d);
>  
> -    for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> +    for (i = 0; i < l; val >>= 8, ++i) {
>          uint8_t wmask = d->wmask[addr + i];
>          uint8_t w1cmask = d->w1cmask[addr + i];
>          assert(!(wmask & w1cmask));
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 728e2d4..bfdc321 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -47,17 +47,33 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
>      return pci_find_device(bus, bus_num, devfn);
>  }
>  
> +void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr,
> +                             uint32_t limit, uint32_t val, uint32_t len)
> +{
> +    assert(len <= 4);
> +    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
> +}
> +
> +uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> +                                uint32_t limit, uint32_t len)
> +{
> +    assert(len <= 4);
> +    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
> +}
> +
>  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)
> +    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_dev->config_write(pci_dev, config_addr, val, len);
> +    pci_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, val,
> +                            len);
>  }
>  
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> @@ -66,12 +82,12 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
>  
> -    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
>          return ~0x0;
>      }
>  
> -    val = pci_dev->config_read(pci_dev, config_addr, len);
> +    val = pci_config_read_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
> +                                 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 0a58595..e95db6c 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -39,6 +39,12 @@ struct PCIHostState {
>      PCIBus *bus;
>  };
>  
> +/* common internal helpers for PCI/PCIe hosts, cut off overflows */
> +void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr,
> +                             uint32_t addr_mask, uint32_t val, uint32_t len);
> +uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> +                                uint32_t addr_mask, 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 b749865..ed6656b 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s,
>  {
>      PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>  
> -    if (!pci_dev)
> +    if (!pci_dev) {
>          return;
> -
> -    pci_dev->config_write(pci_dev,
> -                          PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
> +    }
> +    pci_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr),
> +                            PCIE_CONFIG_SPACE_SIZE, val, len);
>  }
>  
>  static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
>  {
>      PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr);
>  
> -    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
>          return ~0x0;
>      }
> -    return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
> +    return pci_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr),
> +                                  PCIE_CONFIG_SPACE_SIZE, len);

Doesn't this one need to be pci_config_size(pci_dev)?
We can have pci devices on an express root complex
or behind an express to pci bridge.

>  }
>  
>  static void pcie_mmcfg_data_writeb(void *opaque,
> -- 
> 1.7.3.4

Patch

diff --git a/hw/pci.c b/hw/pci.c
index b904a4e..ef94739 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1108,8 +1108,7 @@  uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len)
 {
     uint32_t val = 0;
-    assert(len == 1 || len == 2 || len == 4);
-    len = MIN(len, pci_config_size(d) - address);
+
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
 }
@@ -1117,9 +1116,8 @@  uint32_t pci_default_read_config(PCIDevice *d,
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
     int i, was_irq_disabled = pci_irq_disabled(d);
-    uint32_t config_size = pci_config_size(d);
 
-    for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
+    for (i = 0; i < l; val >>= 8, ++i) {
         uint8_t wmask = d->wmask[addr + i];
         uint8_t w1cmask = d->w1cmask[addr + i];
         assert(!(wmask & w1cmask));
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 728e2d4..bfdc321 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -47,17 +47,33 @@  static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
     return pci_find_device(bus, bus_num, devfn);
 }
 
+void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr,
+                             uint32_t limit, uint32_t val, uint32_t len)
+{
+    assert(len <= 4);
+    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
+}
+
+uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr,
+                                uint32_t limit, uint32_t len)
+{
+    assert(len <= 4);
+    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+}
+
 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)
+    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_dev->config_write(pci_dev, config_addr, val, len);
+    pci_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, val,
+                            len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
@@ -66,12 +82,12 @@  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
-    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
         return ~0x0;
     }
 
-    val = pci_dev->config_read(pci_dev, config_addr, len);
+    val = pci_config_read_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
+                                 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 0a58595..e95db6c 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -39,6 +39,12 @@  struct PCIHostState {
     PCIBus *bus;
 };
 
+/* common internal helpers for PCI/PCIe hosts, cut off overflows */
+void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr,
+                             uint32_t addr_mask, uint32_t val, uint32_t len);
+uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr,
+                                uint32_t addr_mask, 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 b749865..ed6656b 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -57,22 +57,22 @@  static void pcie_mmcfg_data_write(PCIBus *s,
 {
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
 
-    if (!pci_dev)
+    if (!pci_dev) {
         return;
-
-    pci_dev->config_write(pci_dev,
-                          PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
+    }
+    pci_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr),
+                            PCIE_CONFIG_SPACE_SIZE, val, len);
 }
 
 static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr);
 
-    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
         return ~0x0;
     }
-    return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
+    return pci_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr),
+                                  PCIE_CONFIG_SPACE_SIZE, len);
 }
 
 static void pcie_mmcfg_data_writeb(void *opaque,