Re: [Qemu-devel] [PATCH 3/4] pci: Make bounds checks on config space accesses actually work

[David Gibson <david@gibson.dropbear.id.au>]

On Wed, Jan 11, 2012 at 07:25:52AM +0100, Stefan Weil wrote:
> Am 11.01.2012 06:44, schrieb David Gibson:
> >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. 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.
> >
> >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 <david@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)
> 
> Some people use QEMU to detect this kind of errors in an emulated
> systems. Therefore an additional debug output would be good here.
> 
> As long as there is no QEMU standard for reporting this kind of errors,
> a PCI_DPRINTF statement might be best.

This seems like an independent enhancement to me.

-- 
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