* [Qemu-devel] [PATCH] pci: Common overflow prevention
@ 2011-07-21 16:50 Jan Kiszka
2011-07-22 5:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 11+ messages in thread
From: Jan Kiszka @ 2011-07-21 16:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Isaku Yamahata, qemu-devel
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.kiszka@siemens.com>
---
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);
}
static void pcie_mmcfg_data_writeb(void *opaque,
--
1.7.3.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: Common overflow prevention
2011-07-21 16:50 [Qemu-devel] [PATCH] pci: Common overflow prevention Jan Kiszka
@ 2011-07-22 5:32 ` Michael S. Tsirkin
2011-07-22 9:05 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2011-07-22 5:32 UTC (permalink / raw)
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 <jan.kiszka@siemens.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-22 5:32 ` Michael S. Tsirkin
@ 2011-07-22 9:05 ` Jan Kiszka
2011-07-25 15:12 ` Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-07-22 9:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Isaku Yamahata, qemu-devel
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.kiszka@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=%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,
--
1.7.3.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-22 9:05 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
@ 2011-07-25 15:12 ` Michael S. Tsirkin
2011-07-25 15:17 ` Michael S. Tsirkin
2011-07-28 7:23 ` Isaku Yamahata
2 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2011-07-25 15:12 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Isaku Yamahata, qemu-devel
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.kiszka@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=%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);
> +
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 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,
> --
> 1.7.3.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-22 9:05 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
2011-07-25 15:12 ` Michael S. Tsirkin
@ 2011-07-25 15:17 ` Michael S. Tsirkin
2011-07-25 15:18 ` Jan Kiszka
2011-07-28 7:23 ` Isaku Yamahata
2 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2011-07-25 15:17 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Isaku Yamahata, qemu-devel
> 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.kiszka@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=%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,
> --
> 1.7.3.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-25 15:17 ` Michael S. Tsirkin
@ 2011-07-25 15:18 ` Jan Kiszka
0 siblings, 0 replies; 11+ messages in thread
From: Jan Kiszka @ 2011-07-25 15:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Isaku Yamahata, qemu-devel
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.kiszka@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-22 9:05 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
2011-07-25 15:12 ` Michael S. Tsirkin
2011-07-25 15:17 ` Michael S. Tsirkin
@ 2011-07-28 7:23 ` Isaku Yamahata
2011-07-28 8:40 ` Michael S. Tsirkin
2 siblings, 1 reply; 11+ messages in thread
From: Isaku Yamahata @ 2011-07-28 7:23 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel, Michael S. Tsirkin
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.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@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 <yamahata@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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-28 7:23 ` Isaku Yamahata
@ 2011-07-28 8:40 ` Michael S. Tsirkin
2011-07-28 12:50 ` Isaku Yamahata
2011-07-29 1:01 ` Isaku Yamahata
0 siblings, 2 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2011-07-28 8:40 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Jan Kiszka, qemu-devel
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.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@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 <yamahata@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-28 8:40 ` Michael S. Tsirkin
@ 2011-07-28 12:50 ` Isaku Yamahata
2011-07-29 1:01 ` Isaku Yamahata
1 sibling, 0 replies; 11+ messages in thread
From: Isaku Yamahata @ 2011-07-28 12:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jan Kiszka, qemu-devel
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.yamahata@valinux.co.jp>
> > From: Isaku Yamahata <yamahata@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 <yamahata@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-28 8:40 ` Michael S. Tsirkin
2011-07-28 12:50 ` Isaku Yamahata
@ 2011-07-29 1:01 ` Isaku Yamahata
2011-07-29 5:03 ` Michael S. Tsirkin
1 sibling, 1 reply; 11+ messages in thread
From: Isaku Yamahata @ 2011-07-29 1:01 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jan Kiszka, qemu-devel
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.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@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 <yamahata@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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2] pci: Common overflow prevention
2011-07-29 1:01 ` Isaku Yamahata
@ 2011-07-29 5:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2011-07-29 5:03 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Jan Kiszka, qemu-devel
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.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@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 <yamahata@valinux.co.jp>
I tweaked the commit log and applied this.
Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-07-29 5:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-21 16:50 [Qemu-devel] [PATCH] pci: Common overflow prevention Jan Kiszka
2011-07-22 5:32 ` Michael S. Tsirkin
2011-07-22 9:05 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
2011-07-25 15:12 ` Michael S. Tsirkin
2011-07-25 15:17 ` Michael S. Tsirkin
2011-07-25 15:18 ` Jan Kiszka
2011-07-28 7:23 ` Isaku Yamahata
2011-07-28 8:40 ` Michael S. Tsirkin
2011-07-28 12:50 ` Isaku Yamahata
2011-07-29 1:01 ` Isaku Yamahata
2011-07-29 5:03 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.