From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RkrdH-0004qA-RC for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:26:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RkrdE-0005vt-W2 for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:25:59 -0500 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:58276) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RkrdE-0005vo-NW for qemu-devel@nongnu.org; Wed, 11 Jan 2012 01:25:56 -0500 Message-ID: <4F0D2B70.3060609@weilnetz.de> Date: Wed, 11 Jan 2012 07:25:52 +0100 From: Stefan Weil MIME-Version: 1.0 References: <1326260692-7272-1-git-send-email-david@gibson.dropbear.id.au> <1326260692-7272-4-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1326260692-7272-4-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] 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: agraf@suse.de, qemu-devel@nongnu.org Am 11.01.2012 06:44, schrieb David Gibson: > 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 > --- > 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) Some people use QEMU to detect this kind of errors in an emulated systems. Therefore an additional debug output would be good here. As long as there is no QEMU standard for reporting this kind of errors, a PCI_DPRINTF statement might be best. Regards, Stefan Weil