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

[Maxim Davydov]

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