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

[Don Dutile <ddutile@redhat.com>]

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);