From patchwork Fri Jul 22 09:05:01 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kiszka X-Patchwork-Id: 106242 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 25631B6F64 for ; Fri, 22 Jul 2011 19:05:20 +1000 (EST) Received: from localhost ([::1]:57796 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkBfY-0000wl-VL for incoming@patchwork.ozlabs.org; Fri, 22 Jul 2011 05:05:16 -0400 Received: from eggs.gnu.org ([140.186.70.92]:43474) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkBfR-0000u5-Ov for qemu-devel@nongnu.org; Fri, 22 Jul 2011 05:05:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QkBfQ-00063p-AD for qemu-devel@nongnu.org; Fri, 22 Jul 2011 05:05:09 -0400 Received: from goliath.siemens.de ([192.35.17.28]:25033) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QkBfP-000620-Qq for qemu-devel@nongnu.org; Fri, 22 Jul 2011 05:05:08 -0400 Received: from mail1.siemens.de (localhost [127.0.0.1]) by goliath.siemens.de (8.13.6/8.13.6) with ESMTP id p6M9521m009749; Fri, 22 Jul 2011 11:05:05 +0200 Received: from mchn199C.mchp.siemens.de ([139.25.109.49]) by mail1.siemens.de (8.13.6/8.13.6) with ESMTP id p6M9510A013890; Fri, 22 Jul 2011 11:05:01 +0200 Message-ID: <4E293D3D.8070904@siemens.com> Date: Fri, 22 Jul 2011 11:05:01 +0200 From: Jan Kiszka User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 MIME-Version: 1.0 To: "Michael S. Tsirkin" References: <4E2858C2.5050909@siemens.com> <20110722052707.GA8241@redhat.com> In-Reply-To: <20110722052707.GA8241@redhat.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6, seldom 2.4 (older, 4) X-Received-From: 192.35.17.28 Cc: Isaku Yamahata , qemu-devel Subject: [Qemu-devel] [PATCH v2] pci: Common overflow prevention 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 On 2011-07-22 07:32, Michael S. Tsirkin wrote: >> diff --git a/hw/pcie_host.c b/hw/pcie_host.c >> index b749865..ed6656b 100644 >> --- a/hw/pcie_host.c >> +++ b/hw/pcie_host.c >> @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s, >> { >> PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); >> >> - if (!pci_dev) >> + if (!pci_dev) { >> return; >> - >> - pci_dev->config_write(pci_dev, >> - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len); >> + } >> + pci_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), >> + PCIE_CONFIG_SPACE_SIZE, val, len); >> } >> >> static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) >> { >> PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr); >> >> - assert(len == 1 || len == 2 || len == 4); >> if (!pci_dev) { >> return ~0x0; >> } >> - return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len); >> + return pci_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), >> + PCIE_CONFIG_SPACE_SIZE, len); > > Doesn't this one need to be pci_config_size(pci_dev)? > We can have pci devices on an express root complex > or behind an express to pci bridge. Yep, right. Thanks, Jan ------8<------ Introduce pci_config_read/write_common helpers to prevent passing accesses down the callback chain that go beyond the config space limits. Adjust length assertions as they are no longer correct (cutting may generate valid 3 byte accesses). Signed-off-by: Jan Kiszka --- hw/pci.c | 6 ++---- hw/pci_host.c | 24 ++++++++++++++++++++---- hw/pci_host.h | 6 ++++++ hw/pcie_host.c | 12 ++++++------ 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/hw/pci.c b/hw/pci.c index b904a4e..ef94739 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1108,8 +1108,7 @@ uint32_t pci_default_read_config(PCIDevice *d, uint32_t address, int len) { uint32_t val = 0; - assert(len == 1 || len == 2 || len == 4); - len = MIN(len, pci_config_size(d) - address); + memcpy(&val, d->config + address, len); return le32_to_cpu(val); } @@ -1117,9 +1116,8 @@ uint32_t pci_default_read_config(PCIDevice *d, void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l) { int i, was_irq_disabled = pci_irq_disabled(d); - uint32_t config_size = pci_config_size(d); - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) { + for (i = 0; i < l; val >>= 8, ++i) { uint8_t wmask = d->wmask[addr + i]; uint8_t w1cmask = d->w1cmask[addr + i]; assert(!(wmask & w1cmask)); diff --git a/hw/pci_host.c b/hw/pci_host.c index 728e2d4..bfdc321 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -47,17 +47,33 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) return pci_find_device(bus, bus_num, devfn); } +void pci_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)); +} + +uint32_t pci_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)); +} + 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) + 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_dev->config_write(pci_dev, config_addr, val, len); + pci_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, val, + len); } uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) @@ -66,12 +82,12 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); uint32_t val; - assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { return ~0x0; } - val = pci_dev->config_read(pci_dev, config_addr, len); + val = pci_config_read_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE, + len); PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", __func__, pci_dev->name, config_addr, val, len); diff --git a/hw/pci_host.h b/hw/pci_host.h index 0a58595..e95db6c 100644 --- a/hw/pci_host.h +++ b/hw/pci_host.h @@ -39,6 +39,12 @@ struct PCIHostState { PCIBus *bus; }; +/* common internal helpers for PCI/PCIe hosts, cut off overflows */ +void pci_config_write_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t addr_mask, uint32_t val, uint32_t len); +uint32_t pci_config_read_common(PCIDevice *pci_dev, uint32_t addr, + uint32_t addr_mask, 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 b749865..699a53a 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s, { PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr); - if (!pci_dev) + if (!pci_dev) { return; - - pci_dev->config_write(pci_dev, - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len); + } + pci_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), + pci_config_size(pci_dev), val, len); } static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) { PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr); - assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { return ~0x0; } - return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len); + return pci_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), + pci_config_size(pci_dev), len); } static void pcie_mmcfg_data_writeb(void *opaque,