Re: [PATCH v1 2/9] pci: add null-pointer check

[Maxim Davydov <maxim.davydov@openvz.org>]

On 3/30/22 14:07, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2022 00:15, Maxim Davydov wrote:
>> Call pci_bus_get_w64_range can fail with the segmentation fault. For
>> example, this can happen during attempt to get pci-hole64-end 
>> immediately
>> after initialization.
>
> So, immediately after initialization, h->bus is NULL?
>
> The significant bit is, is the value which we calculate without h->bus 
> is correct or not? That should be covered by commit message.
For example, object_new_with_class() returns only initialized object 
(after calling instance_init). It means that pci_root_bus_new() in 
q35_host_realize() hasn't been called for returned object and pci->bus 
== NULL. So, if then we try to call q35_host_get_pci_hole64_end() it 
will fail with segmentation fault in the pci_for_each_device_under_bus() 
(d = bus->devices[devfn], but bus == NULL). Similarly for i440fx. I'm 
not sure that it's the correct behavior.
To reproduce this situation, run "{'execute' : 'query-init-properties'}" 
or qmp_query_init_properties() from 8th patch of this series without 
applying fixes for pci-host.
After this fix, the behavior is the similar as if range_is_empty(&w64) 
== True, but without SEGFAULT. Although, we can check flag 
DeviceState.realized to detect unrealized device.
>
>>
>> Signed-off-by: Maxim Davydov <maxim.davydov@openvz.org>
>> ---
>>   hw/pci-host/i440fx.c | 17 +++++++++++------
>>   hw/pci-host/q35.c    | 17 +++++++++++------
>>   2 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
>> index e08716142b..71a114e551 100644
>> --- a/hw/pci-host/i440fx.c
>> +++ b/hw/pci-host/i440fx.c
>> @@ -158,10 +158,12 @@ static uint64_t 
>> i440fx_pcihost_get_pci_hole64_start_value(Object *obj)
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>>       Range w64;
>> -    uint64_t value;
>> +    uint64_t value = 0;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    }
>>       if (!value && s->pci_hole64_fix) {
>>           value = pc_pci_hole64_start();
>>       }
>> @@ -191,10 +193,13 @@ static void 
>> i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
>>       I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
>>       uint64_t hole64_start = 
>> i440fx_pcihost_get_pci_hole64_start_value(obj);
>>       Range w64;
>> -    uint64_t value, hole64_end;
>> +    uint64_t value = 0;
>> +    uint64_t hole64_end;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    }
>>       hole64_end = ROUND_UP(hole64_start + s->pci_hole64_size, 1ULL 
>> << 30);
>>       if (s->pci_hole64_fix && value < hole64_end) {
>>           value = hole64_end;
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index ab5a47aff5..d679fd85ef 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -124,10 +124,12 @@ static uint64_t 
>> q35_host_get_pci_hole64_start_value(Object *obj)
>>       PCIHostState *h = PCI_HOST_BRIDGE(obj);
>>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>       Range w64;
>> -    uint64_t value;
>> +    uint64_t value = 0;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_lob(&w64);
>> +    }
>>       if (!value && s->pci_hole64_fix) {
>>           value = pc_pci_hole64_start();
>>       }
>> @@ -157,10 +159,13 @@ static void q35_host_get_pci_hole64_end(Object 
>> *obj, Visitor *v,
>>       Q35PCIHost *s = Q35_HOST_DEVICE(obj);
>>       uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj);
>>       Range w64;
>> -    uint64_t value, hole64_end;
>> +    uint64_t value = 0;
>> +    uint64_t hole64_end;
>>   -    pci_bus_get_w64_range(h->bus, &w64);
>> -    value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    if (h->bus) {
>> +        pci_bus_get_w64_range(h->bus, &w64);
>> +        value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1;
>> +    }
>>       hole64_end = ROUND_UP(hole64_start + s->mch.pci_hole64_size, 
>> 1ULL << 30);
>>       if (s->pci_hole64_fix && value < hole64_end) {
>>           value = hole64_end;
>
>

-- 
Best regards,
Maxim Davydov