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<[email protected]>
>
> 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)
>
