Re: [PATCH v2 2/2] pci: ensure configuration access is within bounds

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 6/4/20 12:13 AM, BALATON Zoltan wrote:
> On Thu, 4 Jun 2020, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> While reading PCI configuration bytes, a guest may send an
>> address towards the end of the configuration space. It may lead
>> to an OOB access issue. Assert that 'address + len' is within
>> PCI configuration space.
>>
>> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>> hw/pci/pci.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> Update v2: assert PCI configuration access is within bounds
>>  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00711.html
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 70c66965f5..173bec4fd5 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1381,6 +1381,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
>> {
>>     uint32_t val = 0;
>>
>> +    assert(address + len <= pci_config_size(d));
> 
> Does this allow guest now to crash QEMU? I think it was suggested that
> assert should only be used for cases that can only arise from a
> programming error and not from values set by the guest. If this is
> considered to be an error now to call this function with wrong
> parameters did you check other callers? I've found a few such as:
> 
> hw/scsi/esp-pci.c
> hw/watchdog/wdt_i6300esb.c
> hw/ide/cmd646.c
> hw/vfio/pci.c
> 
> and maybe others. Would it be better to not crash just log invalid
> access and either fix up parameters or return some garbage like 0?

Yes, maybe I was not clear while reviewing v1, we need to audit the
callers and fix them first, then we can safely add the assert here.

> 
> Regards,
> BALATON Zoltan
> 
>> +
>>     if (pci_is_express_downstream_port(d) &&
>>         ranges_overlap(address, len, d->exp.exp_cap + PCI_EXP_LNKSTA,
>> 2)) {
>>         pcie_sync_bridge_lnk(d);
>>