On Thu, Jan 12, 2012 at 04:46:22PM +1100, David Gibson wrote: > The pci_host_config_{read,write}_common() functions perform PCI config > accesses. They take a limit parameter which they appear to be supposed > to bounds check against, however the bounds checking logic, such as it is, > is completely broken. > > Currently, it takes the minimum of the supplied length and the remaining > space in the region and passes that as the length to the underlying > config_{read,write} function pointer. This means that accesses which > partially overrun the region will be silently truncated - which makes > little sense.
Why does it make little sense? Makes sense to me. > Accesses which entirely overrun the region will *not* > be blocked (an exploitable bug) >, because in that case (limit - addr) will > be negative and so the unsigned MIN will always return len instead. Even > if signed arithmetic was used, the config_{read,write} callback wouldn't > know what to do with a negative len parameter. The assumption was callers never pass in such values. Could you please give an example how this exploitable bug can get triggered? > > This patch handles things more sanely by simply ignoring writes which > overrun, and returning -1 for reads, which is the usual hardware convention > for reads to unpopulated IO regions. > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > --- > hw/pci_host.c | 10 ++++++++-- > 1 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/hw/pci_host.c b/hw/pci_host.c > index 44c6c20..16b3ac3 100644 > --- a/hw/pci_host.c > +++ b/hw/pci_host.c > @@ -51,14 +51,20 @@ void pci_host_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)); > + if ((addr + len) <= limit) { > + 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) > { > assert(len <= 4); > - return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); > + if ((addr + len) <= limit) { > + 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) > -- > 1.7.7.3 >