On Tue, 21 Jun 2016 11:57:29 +0300 Marcel Apfelbaum <[email protected]> wrote:
> On 06/17/2016 12:04 PM, Igor Mammedov wrote: > > On Tue, 31 May 2016 20:48:35 +0300 > > Marcel Apfelbaum <[email protected]> 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 <[email protected]> > >> --- > >> 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? > > > 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, > > > >
