From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48148) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFKCW-0003hr-23 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:50:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFKCS-0000m8-SN for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:50:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45912) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFKCS-0000m4-Dr for qemu-devel@nongnu.org; Tue, 21 Jun 2016 07:50:36 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F405F711F0 for ; Tue, 21 Jun 2016 11:50:35 +0000 (UTC) References: <1464716918-29689-1-git-send-email-marcel@redhat.com> <1464716918-29689-5-git-send-email-marcel@redhat.com> <20160617110457.1bd2fa8e@nial.brq.redhat.com> <57690179.9010009@redhat.com> <20160621131918.71d87c63@igors-macbook-pro.local> From: Marcel Apfelbaum Message-ID: <57692A09.9020500@redhat.com> Date: Tue, 21 Jun 2016 14:50:33 +0300 MIME-Version: 1.0 In-Reply-To: <20160621131918.71d87c63@igors-macbook-pro.local> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 4/7] hw/apci: fix pcihp io initialization List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, mst@redhat.com On 06/21/2016 02:19 PM, Igor Mammedov wrote: > On Tue, 21 Jun 2016 11:57:29 +0300 > Marcel Apfelbaum wrote: > >> On 06/17/2016 12:04 PM, Igor Mammedov wrote: >>> On Tue, 31 May 2016 20:48:35 +0300 >>> Marcel Apfelbaum 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 >>>> --- >>>> 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, >>> >> >> >