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

[Michael S. Tsirkin]

On Thu, Jan 12, 2012 at 02:19:59PM +0100, Alexander Graf wrote:
> On 01/12/2012 06:46 AM, 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.  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<address@hidden>
> 
> Michael, please ack or apply yourself. I'll cache this in my tree
> regardless so it doesn't get lost.
> 
> 
> Alex

I'd like to understand the bug a bit.

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