From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlDsb-0002wk-12 for qemu-devel@nongnu.org; Thu, 12 Jan 2012 01:11:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RlDsZ-0005rD-Pg for qemu-devel@nongnu.org; Thu, 12 Jan 2012 01:11:16 -0500 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:58209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlDsZ-0005qz-Ez for qemu-devel@nongnu.org; Thu, 12 Jan 2012 01:11:15 -0500 Message-ID: <4F0E7965.4020604@weilnetz.de> Date: Thu, 12 Jan 2012 07:10:45 +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> <4F0D2B70.3060609@weilnetz.de> <20120112032743.GS4935@truffala.fritz.box> In-Reply-To: <20120112032743.GS4935@truffala.fritz.box> Content-Type: text/plain; charset=ISO-8859-1; 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 , anthony@codemonkey.ws, agraf@suse.de, qemu-devel@nongnu.org Am 12.01.2012 04:27, schrieb David Gibson: > On Wed, Jan 11, 2012 at 07:25:52AM +0100, Stefan Weil wrote: >> 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. > This seems like an independent enhancement to me. I don't mind if the debug output is added in a separate patch. Adding a TODO comment now helps to remember that there is something to be done later. A TODO comment would also be good for PATCH 1/4. Another point: instead of ~0, you could also use UINT32_MAX. Regards, Stefan Weil