On Mon, Apr 16, 2012 at 02:16:24PM +1000, David Gibson wrote: > 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.
./hw/pcie_host.c does not do this. > 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 <m...@redhat.com> > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > --- > 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