From patchwork Mon Mar 19 04:58:11 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Gibson X-Patchwork-Id: 147464 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id A9342B6FE2 for ; Mon, 19 Mar 2012 15:59:44 +1100 (EST) Received: from localhost ([::1]:58438 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Uh4-0008CC-In for incoming@patchwork.ozlabs.org; Mon, 19 Mar 2012 00:59:42 -0400 Received: from eggs.gnu.org ([208.118.235.92]:34603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Ugx-0008C3-8T for qemu-devel@nongnu.org; Mon, 19 Mar 2012 00:59:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9Ugu-0005b3-F1 for qemu-devel@nongnu.org; Mon, 19 Mar 2012 00:59:34 -0400 Received: from ozlabs.org ([203.10.76.45]:37394) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9Ugt-0005an-R2 for qemu-devel@nongnu.org; Mon, 19 Mar 2012 00:59:32 -0400 Received: by ozlabs.org (Postfix, from userid 1007) id DB298B6FE2; Mon, 19 Mar 2012 15:59:29 +1100 (EST) From: David Gibson To: mst@redhat.com Date: Mon, 19 Mar 2012 15:58:11 +1100 Message-Id: <1332133091-7782-1-git-send-email-david@gibson.dropbear.id.au> X-Mailer: git-send-email 1.7.9.1 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 203.10.76.45 Cc: qemu-devel@nongnu.org, David Gibson Subject: [Qemu-devel] [PATCH] pci: Factor out bounds checking on config space accesses X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org There are several paths into the code to emulate PCI config space accesses: one for MMIO to a plain old PCI bridge one for MMIO to a PCIe bridge and one for the pseries machine which provides para-virtualized access to PCI config space. Each of these functions does their own bounds checking against the size of config space to check for addresses outside the size of config space. The pci_host_config_{read,write}_common() (sort of) checks for partial overruns, that is where the address is within the size of config space, but address + length is not, it takes a limit parameter for this purpose. As well as being a small code duplication, and it being weird to separate the checks for partial and total overruns, this checking currently has a few buglets: * For non PCI-Express we assume that the size of config space is PCI_CONFIG_SPACE_SIZE. That's true for everything we emulate now, but is not necessarily true (e.g. PCI-X devices can have extended config space) * The limit parameter is not necessary, since the size of config space can be obtained using pci_config_size() * Partial overruns could only occur with a misaligned access, which should have already been dealt with by this point * Partial overruns are handled as a partial read or write, which is very unlikely behaviour for real hardware * Furthermore, partial reads are 0x0 padded, whereas returning 0xff for unimplemented addresses us much more common. * The partial reads/writes only work correctly by assuming little-endian byte layout. While that is always true for PCI config space, it's an awfully subtle thing to rely on without comment. This patch, therefore, moves the bounds checking wholly into pci_host_config_{read,write}_common(). No partial reads or writes are performed, instead any out-of-bounds write is simply ignored and an out-of-bounds read returns 0xff. This simplifies all the callers, and makes the overall semantics saner for edge cases. Cc: Michael S. Tsirkin Signed-off-by: David Gibson --- hw/pci_host.c | 26 ++++++++++++++------------ hw/pci_host.h | 4 ++-- hw/pcie_host.c | 18 ++---------------- hw/spapr_pci.c | 27 ++++----------------------- 4 files changed, 22 insertions(+), 53 deletions(-) diff --git a/hw/pci_host.c b/hw/pci_host.c index 44c6c20..829d797 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -48,48 +48,50 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) } void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, - uint32_t limit, uint32_t val, uint32_t len) + uint32_t val, uint32_t len) { assert(len <= 4); - pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr)); + if ((addr + len) <= pci_config_size(pci_dev)) { + 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) + uint32_t len) { assert(len <= 4); - return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr)); + if ((addr + len) <= pci_config_size(pci_dev)) { + 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) { PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); if (!pci_dev) { return; } PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n", - __func__, pci_dev->name, config_addr, val, len); - pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, - val, len); + __func__, pci_dev->name, addr, val, len); + pci_host_config_write_common(pci_dev, addr, val, len); } uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) { PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); uint32_t val; if (!pci_dev) { return ~0x0; } - val = pci_host_config_read_common(pci_dev, config_addr, - PCI_CONFIG_SPACE_SIZE, len); + val = pci_host_config_read_common(pci_dev, addr, len); PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", - __func__, pci_dev->name, config_addr, val, len); + __func__, pci_dev->name, addr, val, len); return val; } diff --git a/hw/pci_host.h b/hw/pci_host.h index 359e38f..4bb0838 100644 --- a/hw/pci_host.h +++ b/hw/pci_host.h @@ -42,9 +42,9 @@ struct PCIHostState { /* common internal helpers for PCI/PCIe hosts, cut off overflows */ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr, - uint32_t limit, uint32_t val, uint32_t len); + uint32_t val, uint32_t len); uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, - uint32_t limit, uint32_t len); + uint32_t len); void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len); uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len); diff --git a/hw/pcie_host.c b/hw/pcie_host.c index 28bbe72..3af8610 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -60,19 +60,12 @@ static void pcie_mmcfg_data_write(void *opaque, target_phys_addr_t mmcfg_addr, PCIBus *s = e->pci.bus; PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); uint32_t addr; - uint32_t limit; if (!pci_dev) { return; } addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr); - limit = pci_config_size(pci_dev); - if (limit <= addr) { - /* conventional pci device can be behind pcie-to-pci bridge. - 256 <= addr < 4K has no effects. */ - return; - } - pci_host_config_write_common(pci_dev, addr, limit, val, len); + pci_host_config_write_common(pci_dev, addr, val, len); } static uint64_t pcie_mmcfg_data_read(void *opaque, @@ -83,19 +76,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque, PCIBus *s = e->pci.bus; PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); uint32_t addr; - uint32_t limit; if (!pci_dev) { return ~0x0; } addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr); - limit = pci_config_size(pci_dev); - if (limit <= addr) { - /* conventional pci device can be behind pcie-to-pci bridge. - 256 <= addr < 4K has no effects. */ - return ~0x0; - } - return pci_host_config_read_common(pci_dev, addr, limit, len); + return pci_host_config_read_common(pci_dev, addr, len); } static const MemoryRegionOps pcie_mmcfg_ops = { diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c index e7ef551..77f3e52 100644 --- a/hw/spapr_pci.c +++ b/hw/spapr_pci.c @@ -60,25 +60,6 @@ static uint32_t rtas_pci_cfgaddr(uint32_t arg) return ((arg >> 20) & 0xf00) | (arg & 0xff); } -static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr, - uint32_t limit, uint32_t len) -{ - if ((addr + len) <= limit) { - return pci_host_config_read_common(pci_dev, addr, limit, len); - } else { - return ~0x0; - } -} - -static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr, - uint32_t limit, uint32_t val, - uint32_t len) -{ - if ((addr + len) <= limit) { - pci_host_config_write_common(pci_dev, addr, limit, val, len); - } -} - static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr, uint32_t token, uint32_t nargs, target_ulong args, @@ -94,7 +75,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr, } size = rtas_ld(args, 3); addr = rtas_pci_cfgaddr(rtas_ld(args, 0)); - val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size); + val = pci_host_config_read_common(dev, addr, size); rtas_st(rets, 0, 0); rtas_st(rets, 1, val); } @@ -113,7 +94,7 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr, } size = rtas_ld(args, 1); addr = rtas_pci_cfgaddr(rtas_ld(args, 0)); - val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size); + val = pci_host_config_read_common(dev, addr, size); rtas_st(rets, 0, 0); rtas_st(rets, 1, val); } @@ -134,7 +115,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr, val = rtas_ld(args, 4); size = rtas_ld(args, 3); addr = rtas_pci_cfgaddr(rtas_ld(args, 0)); - rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size); + pci_host_config_write_common(dev, addr, val, size); rtas_st(rets, 0, 0); } @@ -153,7 +134,7 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr, val = rtas_ld(args, 2); size = rtas_ld(args, 1); addr = rtas_pci_cfgaddr(rtas_ld(args, 0)); - rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size); + pci_host_config_write_common(dev, addr, val, size); rtas_st(rets, 0, 0); }