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