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

[David Gibson]

Michael,

Any chance of an ack or nack on this one?

On Mon, Mar 19, 2012 at 03:58:11PM +1100, David Gibson wrote:
> 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);
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson