| Message ID | 1334549784-32362-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 <mst@redhat.com> > > Signed-off-by: David Gibson <david@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
On Wed, Apr 25, 2012 at 11:08:12AM +0300, Michael S. Tsirkin wrote: > 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. Uh, yes. I had assumed that the alignment checking was done in the general MMIO accessor paths, but it looks like it isn't.
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); }
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 <mst@redhat.com> Signed-off-by: David Gibson <david@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.