Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/21/2016 02:19 PM, Igor Mammedov wrote:
> On Tue, 21 Jun 2016 11:57:29 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 06/17/2016 12:04 PM, Igor Mammedov wrote:
>>> On Tue, 31 May 2016 20:48:35 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> The pm initialization code assigns the pcihp IO base and length to
>>>> -1 on error, but the later code will assume 0 as invalid value.
>>>>
>>>> Fix it initializing the above value to 0 as expected.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>    hw/i386/acpi-build.c | 10 ++++++----
>>>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 0c329fb..2097c4c 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -124,17 +124,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>>>>        Object *lpc = ich9_lpc_find();
>>>>        Object *obj = NULL;
>>>>        QObject *o;
>>>> +    int pcihp_io_len, pcihp_io_base;
>>>>
>>>>        pm->cpu_hp_io_base = 0;
>>>> -    pm->pcihp_io_base = 0;
>>>> -    pm->pcihp_io_len = 0;
>>> this introduces uninitialized memory access in q35 case
>>>
>>
>> How? We are always checking pcihp_io_len/pcihp_io_base.
> pm->pcihp_io_len is left uninitialized in q35 case
>
> and then following build_dsdt() would cause jump on uninitialized value
>      /* reserve PCIHP resources */
>      if (pm->pcihp_io_len) {
>
>>
>>>>        if (piix) {
>>>>            obj = piix;
>>>>            pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>>>> -        pm->pcihp_io_base =
>>>> +        pcihp_io_base =
>>>>                object_property_get_int(obj,
>>>> ACPI_PCIHP_IO_BASE_PROP, NULL);
>>>> -        pm->pcihp_io_len =
>>>> +        pcihp_io_len =
>>>>                object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP,
>>>> NULL); +
>>>> +        pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 :
>>>> pcihp_io_base;
>>>> +        pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 :
>>>> pcihp_io_len; }
>>>>        if (lpc) {
>>>>            obj = lpc;
>>>
>>> how about something like that:
>>
>> Please see the next patch. It is only  a temporary measure, the next
>> patch initialize it right to
>> ACPI_PCIHP_IO_BASE_PROP/ACPI_PCIHP_IO_LEN_PROP. If the properties are
>> not there, they will be 0, but we are checking this everywhere.
>>
>> Also I'll prefer pm->pcihp_io_len 0 length to mean "unused" because
>> it is used widely. If you still think is a real issue, I'll change
>> this, of course.
> Maybe squash it into the next patch?
>


I can do that. At first I thought to make the change more readable,
but because of your point I'll squash it.

Thanks,
Marcel



>>
>>
>> Thanks,
>> Marcel
>>
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 4f9aec6..d753e25 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -125,7 +125,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>>>
>>>        pm->cpu_hp_io_base = 0;
>>>        pm->pcihp_io_base = 0;
>>> -    pm->pcihp_io_len = 0;
>>> +    pm->pcihp_io_len = UINT16_MAX;
>>>        if (piix) {
>>>            obj = piix;
>>>            pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
>>> @@ -2053,7 +2053,7 @@ build_dsdt(GArray *table_data, BIOSLinker
>>> *linker, g_ptr_array_free(mem_ranges, true);
>>>
>>>        /* reserve PCIHP resources */
>>> -    if (pm->pcihp_io_len) {
>>> +    if (pm->pcihp_io_len != UINT16_MAX) {
>>>            dev = aml_device("PHPR");
>>>            aml_append(dev, aml_name_decl("_HID",
>>> aml_string("PNP0A06"))); aml_append(dev,
>>>
>>
>>
>