From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57691) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCpDP-0005HK-Ql for qemu-devel@nongnu.org; Wed, 28 Mar 2012 05:31:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SCpDH-0000gT-Ff for qemu-devel@nongnu.org; Wed, 28 Mar 2012 05:30:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16264) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCpDH-0000fd-3z for qemu-devel@nongnu.org; Wed, 28 Mar 2012 05:30:43 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2S9Ufcs013178 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 28 Mar 2012 05:30:41 -0400 Received: from redhat.com (vpn-201-78.tlv.redhat.com [10.35.201.78]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id q2S9UdJG027361 for ; Wed, 28 Mar 2012 05:30:40 -0400 Date: Wed, 28 Mar 2012 11:30:56 +0200 From: "Michael S. Tsirkin" Message-ID: <20120328093055.GA6194@redhat.com> References: <1332133091-7782-1-git-send-email-david@gibson.dropbear.id.au> <20120328011152.GH9582@truffala.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120328011152.GH9582@truffala.fritz.box> Subject: Re: [Qemu-devel] [PATCH] pci: Factor out bounds checking on config space accesses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On Wed, Mar 28, 2012 at 12:11:52PM +1100, David Gibson wrote: > Michael, > > Any chance of an ack or nack on this one? > > On Mon, Mar 19, 2012 at 03:58:11PM +1100, David Gibson wrote: > > 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 last point can be addressed by adding a comment? Patch welcome. > > 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 Sorry, I didn't reply because I have no idea whether this patch is correct. Couldn't figure out from the description whether there's a test case where we differ from real hardware in our behaviour. The change affects lots of platforms and there's no mention of which ones were tested. > > --- > > 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); > > } > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson