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

["Michael S. Tsirkin" <mst@redhat.com>]

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