[PATCH 0/2] pci: config space bounds check and correction

[PATCH 0/2] pci: config space bounds check and correction

[Donald Dutile]

Doing device assignement using a PCIe device with it's
PCI Cap structure at offset 0xcc showed a problem in
the default size mapped for this cap-id.

The failure caused a corruption which might have gone unnoticed
otherwise.

So, add a bounds check in pci_add_capability() & fprintf()
to detail which device & cap structure.  While there, adjust
overlap check to prefix output with '0x' so it's clear in output.
Note: bounds check a bit odd looking, but that's because offset & size
      are uint8's and comparing to greater than 256.

Next, fix assigned_device_pci_cap_init() to set the default
size of PCIe Cap structure (cap-id 0x10) to 0x34 instead of 0x3c.
0x34 is default, min, for endpoint device with a cap version of 2.
Algorithm will have to get a bit more complicated if 
non-endpoint (mriov-based switches?) are ever device-assigned.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>

---

Donald Dutile (2):
      pci: correct pci config size default for cap version 2 endpoints
      pci: bounds check offsets into config_map


 hw/device-assignment.c |    4 +++-
 hw/pci.c               |   16 ++++++++++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

-- 

[PATCH 1/2] pci: bounds check offsets into config_map

[Donald Dutile]

Bounds check to avoid walking off config_map[]
and corrupting what is after it.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
---

 hw/pci.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 5deaa04..9a7ff2d 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
         }
     } else {
         int i;
+	uint32_t config_size = pci_config_size(pdev);
+
+	/* ensure don't walk off end of config_map[] */
+	if (offset > (config_size - size)) {
+            fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
+                    "Attempt to add PCI capability 0x%x at offset 0x%x,"
+                    "size 0x%x, which exceeds emulated PCI config space 0x%x\n",
+                    pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
+                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+                    cap_id, offset, size, config_size);
+	    return -EINVAL;
+	}
 
         for (i = offset; i < offset + size; i++) {
             if (pdev->config_map[i]) {
                 fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
-                        "Attempt to add PCI capability %x at offset "
-                        "%x overlaps existing capability %x at offset %x\n",
+                        "Attempt to add PCI capability 0x%x at offset "
+                        "0x%x overlaps existing capability 0x%x at offset 0x%x\n",
                         pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
                         PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
                         cap_id, offset, pdev->config_map[i], i);

Re: [PATCH 1/2] pci: bounds check offsets into config_map

[Michael S. Tsirkin]

On Wed, Jul 20, 2011 at 07:49:34PM -0400, Donald Dutile wrote:
> Bounds check to avoid walking off config_map[]
> and corrupting what is after it.
> 
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
>  hw/pci.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 5deaa04..9a7ff2d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>          }
>      } else {
>          int i;
> +	uint32_t config_size = pci_config_size(pdev);

This is actually slightly wrong: even for express
devices, legacy capabilities can not spill out from
the low 256 bytes: the extended config space has its
own capability list always starting with the express
capability at offset 256.

> +
> +	/* ensure don't walk off end of config_map[] */
> +	if (offset > (config_size - size)) {

I prefer not to have () around math here.


> +            fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> +                    "Attempt to add PCI capability 0x%x at offset 0x%x,"
> +                    "size 0x%x, which exceeds emulated PCI config space 0x%x\n",
> +                    pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> +                    cap_id, offset, size, config_size);
> +	    return -EINVAL;
> +	}
>  
>          for (i = offset; i < offset + size; i++) {
>              if (pdev->config_map[i]) {
>                  fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> -                        "Attempt to add PCI capability %x at offset "
> -                        "%x overlaps existing capability %x at offset %x\n",
> +                        "Attempt to add PCI capability 0x%x at offset "
> +                        "0x%x overlaps existing capability 0x%x at offset 0x%x\n",
>                          pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
>                          PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
>                          cap_id, offset, pdev->config_map[i], i);

Re: [PATCH 1/2] pci: bounds check offsets into config_map

[Don Dutile]

On 07/21/2011 04:11 AM, Michael S. Tsirkin wrote:
> On Wed, Jul 20, 2011 at 07:49:34PM -0400, Donald Dutile wrote:
>> Bounds check to avoid walking off config_map[]
>> and corrupting what is after it.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>> cc: Alex Williamson<alex.williamson@redhat.com>
>> cc: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>
>>   hw/pci.c |   16 ++++++++++++++--
>>   1 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 5deaa04..9a7ff2d 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -2078,12 +2078,24 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>           }
>>       } else {
>>           int i;
>> +	uint32_t config_size = pci_config_size(pdev);
>
> This is actually slightly wrong: even for express
> devices, legacy capabilities can not spill out from
> the low 256 bytes: the extended config space has its
> own capability list always starting with the express
> capability at offset 256.
>
will just remove this checking & put it all in device-assignment.c

>> +
>> +	/* ensure don't walk off end of config_map[] */
>> +	if (offset>  (config_size - size)) {
>
> I prefer not to have () around math here.
>
>
ok.

>> +            fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
>> +                    "Attempt to add PCI capability 0x%x at offset 0x%x,"
>> +                    "size 0x%x, which exceeds emulated PCI config space 0x%x\n",
>> +                    pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
>> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
>> +                    cap_id, offset, size, config_size);
>> +	    return -EINVAL;
>> +	}
>>
>>           for (i = offset; i<  offset + size; i++) {
>>               if (pdev->config_map[i]) {
>>                   fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
>> -                        "Attempt to add PCI capability %x at offset "
>> -                        "%x overlaps existing capability %x at offset %x\n",
>> +                        "Attempt to add PCI capability 0x%x at offset "
>> +                        "0x%x overlaps existing capability 0x%x at offset 0x%x\n",
>>                           pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
>>                           PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
>>                           cap_id, offset, pdev->config_map[i], i);

[PATCH 2/2] pci: correct pci config size default for cap version 2 endpoints

[Donald Dutile]

Corrected default size of PCIe Cap Structure from 0x3c to 0x34.
0x34 is min size for version 2 Cap Structure for an endpoint.

0x3c value failed on a real device that aligned this
structure at 0xcc, exceeding the size of the config_map[]
by 8 bytes.

Signed-off-by: Donald Dutile <ddutile@redhat.com>
cc: Alex Williamson <alex.williamson@redhat.com>
cc: Michael S. Tsirkin <mst@redhat.com>
---

 hw/device-assignment.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 36ad6b0..1d2ee5d 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1422,12 +1422,14 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         uint8_t version;
         uint16_t type, devctl, lnkcap, lnksta;
         uint32_t devcap;
-        int size = 0x3c; /* version 2 size */
 
         version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS);
         version &= PCI_EXP_FLAGS_VERS;
         if (version == 1) {
             size = 0x14;
+	} else if (version == 2) {
+	    /* don't include slot cap/stat/ctrl 2 regs; only support endpoints */
+	    size = 0x34;
         } else if (version > 2) {
             fprintf(stderr, "Unsupported PCI express capability version %d\n",
                     version);

Re: [PATCH 2/2] pci: correct pci config size default for cap version 2 endpoints

[Alex Williamson]

On Wed, 2011-07-20 at 19:51 -0400, Donald Dutile wrote:
> Corrected default size of PCIe Cap Structure from 0x3c to 0x34.
> 0x34 is min size for version 2 Cap Structure for an endpoint.
> 
> 0x3c value failed on a real device that aligned this
> structure at 0xcc, exceeding the size of the config_map[]
> by 8 bytes.
> 
> Signed-off-by: Donald Dutile <ddutile@redhat.com>
> cc: Alex Williamson <alex.williamson@redhat.com>
> cc: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
>  hw/device-assignment.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 36ad6b0..1d2ee5d 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1422,12 +1422,14 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>          uint8_t version;
>          uint16_t type, devctl, lnkcap, lnksta;
>          uint32_t devcap;
> -        int size = 0x3c; /* version 2 size */
>  
>          version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS);
>          version &= PCI_EXP_FLAGS_VERS;
>          if (version == 1) {
>              size = 0x14;
> +	} else if (version == 2) {
> +	    /* don't include slot cap/stat/ctrl 2 regs; only support endpoints */
> +	    size = 0x34;
>          } else if (version > 2) {
>              fprintf(stderr, "Unsupported PCI express capability version %d\n",
>                      version);
> 

Does this compile?  Where is size defined now?  Also whitespace issues
with tabs.

Alex

Re: [PATCH 2/2] pci: correct pci config size default for cap version 2 endpoints

[Don Dutile]

On 07/20/2011 10:43 PM, Alex Williamson wrote:
> On Wed, 2011-07-20 at 19:51 -0400, Donald Dutile wrote:
>> Corrected default size of PCIe Cap Structure from 0x3c to 0x34.
>> 0x34 is min size for version 2 Cap Structure for an endpoint.
>>
>> 0x3c value failed on a real device that aligned this
>> structure at 0xcc, exceeding the size of the config_map[]
>> by 8 bytes.
>>
>> Signed-off-by: Donald Dutile<ddutile@redhat.com>
>> cc: Alex Williamson<alex.williamson@redhat.com>
>> cc: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>
>>   hw/device-assignment.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 36ad6b0..1d2ee5d 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -1422,12 +1422,14 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
>>           uint8_t version;
>>           uint16_t type, devctl, lnkcap, lnksta;
>>           uint32_t devcap;
>> -        int size = 0x3c; /* version 2 size */
>>
>>           version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS);
>>           version&= PCI_EXP_FLAGS_VERS;
>>           if (version == 1) {
>>               size = 0x14;
>> +	} else if (version == 2) {
>> +	    /* don't include slot cap/stat/ctrl 2 regs; only support endpoints */
>> +	    size = 0x34;
>>           } else if (version>  2) {
>>               fprintf(stderr, "Unsupported PCI express capability version %d\n",
>>                       version);
>>
>
> Does this compile?  Where is size defined now?  Also whitespace issues
> with tabs.
>
> Alex
>
doh! during recent workstation upgrade, i forgot to install pciutils-devel;
I didn't look at the configure output closely, and now see this file wasn't compiled!
thanks for catch.  will repost shortly.