Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-28 Thread Isaku Yamahata 
This might be a bit late comment...

On Fri, Jul 22, 2011 at 11:05:01AM +0200, Jan Kiszka wrote:
 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));
 +}
 +

Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  addr.
So we need explicit if (limit  addr) return;.
Here's the patch for pci branch.

From 75c1a2b47c93ad987cd7a37fb62bda9a59f27948 Mon Sep 17 00:00:00 2001
Message-Id: 
75c1a2b47c93ad987cd7a37fb62bda9a59f27948.1311837763.git.yamah...@valinux.co.jp
From: Isaku Yamahata yamah...@valinux.co.jp
Date: Thu, 28 Jul 2011 16:20:28 +0900
Subject: [PATCH] pci/host: limit check of pci_host_config_read/write_common

This patch adds boundary check in pci_host_config_read/write_common()
Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  addr.
So we need explicit if (limit = addr) return;

Signed-off-by: Isaku Yamahata yamah...@valinux.co.jp
---
 hw/pci_host.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 2e8a29f..71fd3a1 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -51,6 +51,9 @@ void pci_host_config_write_common(PCIDevice *pci_dev, 
uint32_t addr,
   uint32_t limit, uint32_t val, uint32_t len)
 {
 assert(len = 4);
+if (limit = addr) {
+return;
+}
 pci_dev-config_write(pci_dev, addr, val, MIN(len, limit - addr));
 }
 
@@ -58,6 +61,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, 
uint32_t addr,
  uint32_t limit, uint32_t len)
 {
 assert(len = 4);
+if (limit = addr) {
+return 0;
+}
 return pci_dev-config_read(pci_dev, addr, MIN(len, limit - addr));
 }
 
-- 
1.7.1.1

-- 
yamahata

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-28 Thread Michael S. Tsirkin 
On Thu, Jul 28, 2011 at 04:23:24PM +0900, Isaku Yamahata wrote:
 This might be a bit late comment...
 
 On Fri, Jul 22, 2011 at 11:05:01AM +0200, Jan Kiszka wrote:
  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));
  +}
  +
 
 Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  
 addr.
 So we need explicit if (limit  addr) return;.
 Here's the patch for pci branch.
 
 From 75c1a2b47c93ad987cd7a37fb62bda9a59f27948 Mon Sep 17 00:00:00 2001
 Message-Id: 
 75c1a2b47c93ad987cd7a37fb62bda9a59f27948.1311837763.git.yamah...@valinux.co.jp
 From: Isaku Yamahata yamah...@valinux.co.jp
 Date: Thu, 28 Jul 2011 16:20:28 +0900
 Subject: [PATCH] pci/host: limit check of pci_host_config_read/write_common
 
 This patch adds boundary check in pci_host_config_read/write_common()
 Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  
 addr.
 So we need explicit if (limit = addr) return;
 
 Signed-off-by: Isaku Yamahata yamah...@valinux.co.jp

I don't see a problem with this, but could you please clarify when does
this happen? I think this is only possible for a pci device
behind an express root. If so, this belongs in pcie_host.c

I'd also like this info to be recorded in the commit log.

 ---
  hw/pci_host.c |6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)
 
 diff --git a/hw/pci_host.c b/hw/pci_host.c
 index 2e8a29f..71fd3a1 100644
 --- a/hw/pci_host.c
 +++ b/hw/pci_host.c
 @@ -51,6 +51,9 @@ void pci_host_config_write_common(PCIDevice *pci_dev, 
 uint32_t addr,
uint32_t limit, uint32_t val, uint32_t len)
  {
  assert(len = 4);
 +if (limit = addr) {
 +return;
 +}
  pci_dev-config_write(pci_dev, addr, val, MIN(len, limit - addr));
  }
  
 @@ -58,6 +61,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, 
 uint32_t addr,
   uint32_t limit, uint32_t len)
  {
  assert(len = 4);
 +if (limit = addr) {
 +return 0;
 +}
  return pci_dev-config_read(pci_dev, addr, MIN(len, limit - addr));
  }
  
 -- 
 1.7.1.1
 
 -- 
 yamahata

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-28 Thread Isaku Yamahata 
On Thu, Jul 28, 2011 at 11:40:21AM +0300, Michael S. Tsirkin wrote:
 On Thu, Jul 28, 2011 at 04:23:24PM +0900, Isaku Yamahata wrote:
  This might be a bit late comment...
  
  On Fri, Jul 22, 2011 at 11:05:01AM +0200, Jan Kiszka wrote:
   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));
   +}
   +
  
  Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  
  addr.
  So we need explicit if (limit  addr) return;.
  Here's the patch for pci branch.
  
  From 75c1a2b47c93ad987cd7a37fb62bda9a59f27948 Mon Sep 17 00:00:00 2001
  Message-Id: 
  75c1a2b47c93ad987cd7a37fb62bda9a59f27948.1311837763.git.yamah...@valinux.co.jp
  From: Isaku Yamahata yamah...@valinux.co.jp
  Date: Thu, 28 Jul 2011 16:20:28 +0900
  Subject: [PATCH] pci/host: limit check of pci_host_config_read/write_common
  
  This patch adds boundary check in pci_host_config_read/write_common()
  Since limit and addr is unsigned, MIN(len, limit - addr) = len if limit  
  addr.
  So we need explicit if (limit = addr) return;
  
  Signed-off-by: Isaku Yamahata yamah...@valinux.co.jp
 
 I don't see a problem with this, but could you please clarify when does
 this happen? I think this is only possible for a pci device
 behind an express root. If so, this belongs in pcie_host.c

Right. I'll move the check into pcie_host.c

-- 
yamahata

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-28 Thread Isaku Yamahata 
On Thu, Jul 28, 2011 at 11:40:21AM +0300, Michael S. Tsirkin wrote:
 I don't see a problem with this, but could you please clarify when does
 this happen? I think this is only possible for a pci device
 behind an express root. If so, this belongs in pcie_host.c
 
 I'd also like this info to be recorded in the commit log.

From 1dd598fd35d4e988dc51487829ed66208ca89021 Mon Sep 17 00:00:00 2001
Message-Id: 
1dd598fd35d4e988dc51487829ed66208ca89021.1311901239.git.yamah...@valinux.co.jp
From: Isaku Yamahata yamah...@valinux.co.jp
Date: Fri, 29 Jul 2011 09:52:45 +0900
Subject: [PATCH] pcie_host: limit check of pcie_mmcfg_data_write/read

This patch adds the check where the offset in the configuration space
is in its configuration size.

MMCFG area allows access of pcie configuration space to be in
offset [0, 4K).
At the same time, conventional pci devices whose configuration space size
is 256 bytes can be behind pcie-to-pci bridge.
The access whose offset is [256, 4K) should have no effect
on the conventional pci device
Add the limit check and ignore such accesses.

Signed-off-by: Isaku Yamahata yamah...@valinux.co.jp
---
 hw/pcie_host.c |   28 ++--
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index f0b3d13..f9fea3d 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -56,23 +56,39 @@ static void pcie_mmcfg_data_write(PCIBus *s,
   uint32_t mmcfg_addr, uint32_t val, int len)
 {
 PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
+uint32_t addr;
+uint32_t limit;
 
 if (!pci_dev) {
 return;
 }
-pci_host_config_write_common(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr),
- pci_config_size(pci_dev), val, len);
+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);
 }
 
-static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
+static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t mmcfg_addr, int len)
 {
-PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr);
+PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
+uint32_t addr;
+uint32_t limit;
 
 if (!pci_dev) {
 return ~0x0;
 }
-return pci_host_config_read_common(pci_dev, PCIE_MMCFG_CONFOFFSET(addr),
-   pci_config_size(pci_dev), len);
+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);
 }
 
 static void pcie_mmcfg_data_writeb(void *opaque,
-- 
1.7.1.1



-- 
yamahata

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-28 Thread Michael S. Tsirkin 
On Fri, Jul 29, 2011 at 10:01:43AM +0900, Isaku Yamahata wrote:
 On Thu, Jul 28, 2011 at 11:40:21AM +0300, Michael S. Tsirkin wrote:
  I don't see a problem with this, but could you please clarify when does
  this happen? I think this is only possible for a pci device
  behind an express root. If so, this belongs in pcie_host.c
  
  I'd also like this info to be recorded in the commit log.
 
 From 1dd598fd35d4e988dc51487829ed66208ca89021 Mon Sep 17 00:00:00 2001
 Message-Id: 
 1dd598fd35d4e988dc51487829ed66208ca89021.1311901239.git.yamah...@valinux.co.jp
 From: Isaku Yamahata yamah...@valinux.co.jp
 Date: Fri, 29 Jul 2011 09:52:45 +0900
 Subject: [PATCH] pcie_host: limit check of pcie_mmcfg_data_write/read
 
 This patch adds the check where the offset in the configuration space
 is in its configuration size.
 
 MMCFG area allows access of pcie configuration space to be in
 offset [0, 4K).
 At the same time, conventional pci devices whose configuration space size
 is 256 bytes can be behind pcie-to-pci bridge.
 The access whose offset is [256, 4K) should have no effect
 on the conventional pci device
 Add the limit check and ignore such accesses.
 
 Signed-off-by: Isaku Yamahata yamah...@valinux.co.jp

I tweaked the commit log and applied this.
Thanks!

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-25 Thread Michael S. Tsirkin 
On Fri, Jul 22, 2011 at 11:05:01AM +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 jan.kis...@siemens.com
 ---
  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=%02PRIx32 val=%08PRIx32 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);
 +

OK, except it's config_size, not addr_mask anymore.
I'd also prefer a name like pci_host_config_write_common/
pci_host_config_read_common but it's not a must.

  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

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-25 Thread Michael S. Tsirkin 
 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 jan.kis...@siemens.com

I renamed to pci_host_config_read/write_common and applied this.
Thanks!

 ---
  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=%02PRIx32 val=%08PRIx32 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) {

Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-25 Thread Jan Kiszka 
On 2011-07-25 17:17, Michael S. Tsirkin 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 jan.kis...@siemens.com
 
 I renamed to pci_host_config_read/write_common and applied this.
 Thanks!

Perfect, thanks for cleaning up!

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH v2] pci: Common overflow prevention

2011-07-22 Thread Jan Kiszka 
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 jan.kis...@siemens.com
---
 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=%02PRIx32 val=%08PRIx32 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