From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35356) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qk8Kz-0001av-4t for qemu-devel@nongnu.org; Fri, 22 Jul 2011 01:31:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qk8Kx-0004y6-QE for qemu-devel@nongnu.org; Fri, 22 Jul 2011 01:31:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12438) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qk8Kx-0004y0-EV for qemu-devel@nongnu.org; Fri, 22 Jul 2011 01:31:47 -0400 Date: Fri, 22 Jul 2011 08:32:14 +0300 From: "Michael S. Tsirkin" Message-ID: <20110722052707.GA8241@redhat.com> References: <4E2858C2.5050909@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4E2858C2.5050909@siemens.com> Subject: Re: [Qemu-devel] [PATCH] pci: Common overflow prevention List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Isaku Yamahata , qemu-devel On Thu, Jul 21, 2011 at 06:50:10PM +0200, Jan Kiszka wrote: > 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 > --- > > Now I have to deal with 3 byte config space access for device > assignment, but Michael was right, such things are possible, even in > PCIe. > > 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..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. > } > > static void pcie_mmcfg_data_writeb(void *opaque, > -- > 1.7.3.4