From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlKm8-0002nc-9E for qemu-devel@nongnu.org; Thu, 12 Jan 2012 08:33:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RlKm2-00016w-MP for qemu-devel@nongnu.org; Thu, 12 Jan 2012 08:33:04 -0500 Date: Thu, 12 Jan 2012 15:32:32 +0200 From: "Michael S. Tsirkin" Message-ID: <20120112133231.GB16328@redhat.com> References: <1326347188-12119-1-git-send-email-david@gibson.dropbear.id.au> <1326347188-12119-4-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1326347188-12119-4-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [PATCH 3/9] pci: Make bounds checks on config space accesses actually work List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org 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 > --- > 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 >