From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlKZg-0006Fr-26 for qemu-devel@nongnu.org; Thu, 12 Jan 2012 08:20:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RlKZY-0007GB-HM for qemu-devel@nongnu.org; Thu, 12 Jan 2012 08:20:12 -0500 Message-ID: <4F0EDDFF.5020904@suse.de> Date: Thu, 12 Jan 2012 14:19:59 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1326347188-12119-1-git-send-email-david@gibson.dropbear.id.au> <1326347188-12119-4-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1326347188-12119-4-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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, qemu-devel@nongnu.org, Michael Tsirkin On 01/12/2012 06:46 AM, 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. 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 Michael, please ack or apply yourself. I'll cache this in my tree regardless so it doesn't get lost. Alex > --- > 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)