From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web11.10334.1581953067742292993 for ; Mon, 17 Feb 2020 07:24:27 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: liming.gao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Feb 2020 07:24:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,453,1574150400"; d="scan'208";a="258298310" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 17 Feb 2020 07:24:26 -0800 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 17 Feb 2020 07:24:26 -0800 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by SHSMSX606.ccr.corp.intel.com (10.109.6.216) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 17 Feb 2020 23:24:24 +0800 Received: from shsmsx606.ccr.corp.intel.com ([10.109.6.216]) by SHSMSX606.ccr.corp.intel.com ([10.109.6.216]) with mapi id 15.01.1713.004; Mon, 17 Feb 2020 23:24:24 +0800 From: "Liming Gao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Ni, Ray" , "Gao, Zhichao" , Sami Mujawar , Matteo Carlini , nd Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Thread-Topic: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Thread-Index: AQHV4z8ODQS1EMenHEmJTXu6P/S/Oqgfgj2QgAABB6CAAAKzAA== Date: Mon, 17 Feb 2020 15:24:24 +0000 Message-ID: References: <20200214135906.34344-1-krzysztof.koch@arm.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-version: 11.2.0.6 dlp-product: dlpe-windows dlp-reaction: no-action x-originating-ip: [10.239.127.36] MIME-Version: 1.0 Return-Path: liming.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Krzysztof: Yes. Please create one BZ for this issue.=20 Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Krzysztof= Koch > Sent: Monday, February 17, 2020 11:23 PM > To: devel@edk2.groups.io; Gao, Liming > Cc: Ni, Ray ; Gao, Zhichao ; Sa= mi Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent inf= inite loop if structure length is 0 >=20 > Hi Liming, >=20 > I haven't created a BZ yet, shall I create one? It would be great if the= patch makes it to the stable tag. >=20 > Over the last few months I added some security features to acpiview. The= y make this debug tool less sensitive to exploits from ACPI > tables. This patch completes my efforts in making the tool more reliable= . >=20 > Kind regards, > Krzysztof >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Liming Ga= o via Groups.Io > Sent: Monday, February 17, 2020 15:11 > To: devel@edk2.groups.io; Krzysztof Koch > Cc: Ni, Ray ; Gao, Zhichao ; Sa= mi Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent inf= inite loop if structure length is 0 >=20 > Krzysztof: > Is there one BZ for this issue? Does this patch catch to this edk2 sta= ble tag 202002? >=20 > Thanks > Liming > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of > > Krzysztof Koch > > Sent: Friday, February 14, 2020 9:59 PM > > To: devel@edk2.groups.io > > Cc: Ni, Ray ; Gao, Zhichao ; > > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > > Subject: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Prevent > > infinite loop if structure length is 0 > > > > Extend validation of ACPI structure lengths which are read from the > > ACPI table being parsed. Additionally check if the structure 'Length' > > field value is positive. If not, stop parsing the faulting table. > > > > Some ACPI tables define internal structures of variable size. The > > 'Length' field inside the substructure is used to update a pointer > > used for table traversal. If the byte-length of the structure is equal > > to 0, acpiview can enter an infinite loop. This condition can occur > > if, for example, the zero-allocated ACPI table buffer is not fully pop= ulated. > > This is typically a bug on the ACPI table writer side. > > > > In short, this method helps acpiview recover gracefully from a > > zero-valued ACPI structure length. > > > > Signed-off-by: Krzysztof Koch > > --- > > > > Changes can be seen at: > > https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_l > > oops_v1 > > > > Notes: > > v1: > > - prevent infinite loops in acpiview parsers [Krzysztof] > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > > | 15 ++++++----- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c > > | 13 ++++----- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c > > | 14 +++++----- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c > > | 28 ++++++-------------- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c > > | 15 ++++++----- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c > > | 14 +++++----- > > 6 files changed, 47 insertions(+), 52 deletions(-) > > > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser > > .c index > > 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c243e > > d78b9f12ee97 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > DBG2 table parser > > > > - Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -282,15 +282,16 @@ ParseAcpiDbg2 ( > > return; > > } > > > > - // Make sure the Debug Device Information structure lies inside t= he table. > > - if ((Offset + *DbgDevInfoLen) > AcpiTableLength) { > > + // Validate Debug Device Information Structure length > > + if ((*DbgDevInfoLen =3D=3D 0) || > > + ((Offset + (*DbgDevInfoLen)) > AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > - L"ERROR: Invalid Debug Device Information structure length. "= \ > > - L"DbgDevInfoLen =3D %d. RemainingTableBufferLength =3D %d. = " \ > > - L"DBG2 parsing aborted.\n", > > + L"ERROR: Invalid Debug Device Information Structure length. "= \ > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *DbgDevInfoLen, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > > .c index > > 699a55b549ec3fa61bbd156898821055dc019199..bdd30ff45c61142c071ead63a27b > > abab8998721b 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > GTDT table parser > > > > - Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -327,15 +327,16 @@ ParseAcpiGtdt ( > > return; > > } > > > > - // Make sure the Platform Timer is inside the table. > > - if ((Offset + *PlatformTimerLength) > AcpiTableLength) { > > + // Validate Platform Timer Structure length > > + if ((*PlatformTimerLength =3D=3D 0) || > > + ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > L"ERROR: Invalid Platform Timer Structure length. " \ > > - L"PlatformTimerLength =3D %d. RemainingTableBufferLength = =3D %d. " \ > > - L"GTDT parsing aborted.\n", > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *PlatformTimerLength, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser > > .c index > > 9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6..9a006a01448b897865cd7cd85651 > > c816933acf05 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > IORT table parser > > > > - Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -687,14 +687,16 @@ ParseAcpiIort ( > > return; > > } > > > > - // Make sure the IORT Node is inside the table > > - if ((Offset + (*IortNodeLength)) > AcpiTableLength) { > > + // Validate IORT Node length > > + if ((*IortNodeLength =3D=3D 0) || > > + ((Offset + (*IortNodeLength)) > AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > - L"ERROR: Invalid IORT node length. IortNodeLength =3D %d. " \ > > - L"RemainingTableBufferLength =3D %d. IORT parsing aborted.\= n", > > + L"ERROR: Invalid IORT Node length. " \ > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *IortNodeLength, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser > > .c index > > 438905cb24f58b8b82e8fe61280e72f765d578d8..f85d2b36532cfc5db36fe7bef983 > > 0cccc64969cc 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > MADT table parser > > > > - Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -273,28 +273,16 @@ ParseAcpiMadt ( > > return; > > } > > > > - // Make sure forward progress is made. > > - if (*MadtInterruptControllerLength < 2) { > > + // Validate Interrupt Controller Structure length > > + if ((*MadtInterruptControllerLength =3D=3D 0) || > > + ((Offset + (*MadtInterruptControllerLength)) > > > + AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > - L"ERROR: Structure length is too small: " \ > > - L"MadtInterruptControllerLength =3D %d. " \ > > - L"MadtInterruptControllerType =3D %d. MADT parsing aborted.= \n", > > + L"ERROR: Invalid Interrupt Controller Structure length. " \ > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *MadtInterruptControllerLength, > > - *MadtInterruptControllerType > > - ); > > - return; > > - } > > - > > - // Make sure the MADT structure lies inside the table > > - if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) = { > > - IncrementErrorCount (); > > - Print ( > > - L"ERROR: Invalid MADT structure length. " \ > > - L"MadtInterruptControllerLength =3D %d. " \ > > - L"RemainingTableBufferLength =3D %d. MADT parsing aborted.\= n", > > - *MadtInterruptControllerLength, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser > > .c index > > 675ba75f02b367cd5ad9f2ac23c30ed0ab58f286..0db272c16af0ad8824c8da4c88dd > > 409c8550112a 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > PPTT table parser > > > > - Copyright (c) 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -425,15 +425,16 @@ ParseAcpiPptt ( > > return; > > } > > > > - // Make sure the PPTT structure lies inside the table > > - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLengt= h) { > > + // Validate Processor Topology Structure length > > + if ((*ProcessorTopologyStructureLength =3D=3D 0) || > > + ((Offset + (*ProcessorTopologyStructureLength)) > > > + AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > - L"ERROR: Invalid PPTT structure length. " \ > > - L"ProcessorTopologyStructureLength =3D %d. " \ > > - L"RemainingTableBufferLength =3D %d. PPTT parsing aborted.\= n", > > + L"ERROR: Invalid Processor Topology Structure length. " \ > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *ProcessorTopologyStructureLength, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser > > .c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser > > .c index > > 3613900ae322483fdd3d3383de4e22ba75b2128b..6f66be68cc0bed14811a0432c61a > > 79fd47c54890 100644 > > --- > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser > > .c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPa > > +++ rser.c > > @@ -1,7 +1,7 @@ > > /** @file > > SRAT table parser > > > > - Copyright (c) 2016 - 2019, ARM Limited. All rights reserved. > > + Copyright (c) 2016 - 2020, ARM Limited. All rights reserved. > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @par Reference(s): > > @@ -412,14 +412,16 @@ ParseAcpiSrat ( > > return; > > } > > > > - // Make sure the SRAT structure lies inside the table > > - if ((Offset + *SratRALength) > AcpiTableLength) { > > + // Validate Static Resource Allocation Structure length > > + if ((*SratRALength =3D=3D 0) || > > + ((Offset + (*SratRALength)) > AcpiTableLength)) { > > IncrementErrorCount (); > > Print ( > > - L"ERROR: Invalid SRAT structure length. SratRALength =3D %d. = " \ > > - L"RemainingTableBufferLength =3D %d. SRAT parsing aborted.\= n", > > + L"ERROR: Invalid Static Resource Allocation Structure length.= " \ > > + L"Length =3D %d. Offset =3D %d. AcpiTableLength =3D %d.\n", > > *SratRALength, > > - AcpiTableLength - Offset > > + Offset, > > + AcpiTableLength > > ); > > return; > > } > > -- > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > >=20 >=20 >=20 >=20 >=20 >=20