From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.1786.1582177936429973338 for ; Wed, 19 Feb 2020 21:52:16 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: zhichao.gao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Feb 2020 21:52:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,463,1574150400"; d="scan'208";a="239917666" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 19 Feb 2020 21:52:15 -0800 Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 19 Feb 2020 21:52:14 -0800 Received: from shsmsx603.ccr.corp.intel.com (10.109.6.143) by SHSMSX603.ccr.corp.intel.com (10.109.6.143) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 20 Feb 2020 13:52:12 +0800 Received: from shsmsx603.ccr.corp.intel.com ([10.109.6.143]) by SHSMSX603.ccr.corp.intel.com ([10.109.6.143]) with mapi id 15.01.1713.004; Thu, 20 Feb 2020 13:52:12 +0800 From: "Gao, Zhichao" To: Krzysztof Koch , "devel@edk2.groups.io" CC: "Ni, Ray" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Thread-Topic: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if structure length is 0 Thread-Index: AQHV5w65Nh8OmfgjDUGpVIfaj1A4e6gjlTxg Date: Thu, 20 Feb 2020 05:52:12 +0000 Message-ID: <546316eb919b4e08a6d5c0c6152e4bb8@intel.com> References: <20200219102338.272-1-krzysztof.koch@arm.com> In-Reply-To: <20200219102338.272-1-krzysztof.koch@arm.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWJiNmM5ZDQtZDk1ZC00ZWIwLWI0NDUtMmZkYWFmN2I4ZjcyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMTJrdVlFYmZ2RXU0VjNIM1U1V1U0MVcyMTBBcm1TelRvXC9ZNlwvNzZQbk93bVBDR0l2M0VteDIxVVlpUzVkSHVDIn0= dlp-reaction: no-action dlp-version: 11.2.0.6 x-originating-ip: [10.239.127.36] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Since there is no change for the V2 patch. It is a good behavior to add the= R-B from V1 to V2. That would make the reviewer aware that there is no cod= e change and review requirement for this patch. I would keep the R-B in V1 = and help to merge the patch. Thanks, Zhichao > -----Original Message----- > From: Krzysztof Koch > Sent: Wednesday, February 19, 2020 6:24 PM > To: devel@edk2.groups.io > Cc: Ni, Ray ; Gao, Zhichao ; > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > Subject: [PATCH v2 1/1] ShellPkg: acpiview: Prevent infinite loop if stru= cture > length is 0 >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2534 >=20 > 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. >=20 > Some ACPI tables define internal structures of variable size. The 'Length= ' field > inside the substructure is used to update a pointer used for table traver= sal. If the > byte-length of the structure is equal to 0, acpiview can enter an infinit= e loop. > This condition can occur if, for example, the zero-allocated ACPI table b= uffer is > not fully populated. > This is typically a bug on the ACPI table writer side. >=20 > In short, this method helps acpiview recover gracefully from a zero-value= d ACPI > structure length. >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_prevent_inf_loop= s > _v2 >=20 > Notes: > v2: > - Add BZ link to the commit message [Zhichao] >=20 > v1: > - prevent infinite loops in acpiview parsers [Krzysztof] >=20 > 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(-) >=20 > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > index > 0f730a306a94329a23fbaf54b59f1833b44616ba..9df111ecaa7d7a703a13a39c24 > 3ed78b9f12ee97 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > DBG2 table parser >=20 > - 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 >=20 > @par Reference(s): > @@ -282,15 +282,16 @@ ParseAcpiDbg2 ( > return; > } >=20 > - // Make sure the Debug Device Information structure lies inside the = 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..bdd30ff45c61142c071ead63a > 27babab8998721b 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParse= r.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > GTDT table parser >=20 > - 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 >=20 > @par Reference(s): > @@ -327,15 +327,16 @@ ParseAcpiGtdt ( > return; > } >=20 > - // 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..9a006a01448b897865cd7cd85 > 651c816933acf05 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParse= r.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortPars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > IORT table parser >=20 > - 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 >=20 > @par Reference(s): > @@ -687,14 +687,16 @@ ParseAcpiIort ( > return; > } >=20 > - // 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..f85d2b36532cfc5db36fe7bef9 > 830cccc64969cc 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtPars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > MADT table parser >=20 > - 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 >=20 > @par Reference(s): > @@ -273,28 +273,16 @@ ParseAcpiMadt ( > return; > } >=20 > - // 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..0db272c16af0ad8824c8da4c88 > dd409c8550112a 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParse= r.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > PPTT table parser >=20 > - Copyright (c) 2019, ARM Limited. All rights reserved. > + Copyright (c) 2019 - 2020, ARM Limited. All rights reserved. > SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > @par Reference(s): > @@ -425,15 +425,16 @@ ParseAcpiPptt ( > return; > } >=20 > - // Make sure the PPTT structure lies inside the table > - if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) = { > + // 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..6f66be68cc0bed14811a0432c > 61a79fd47c54890 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParse= r.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratPars > +++ er.c > @@ -1,7 +1,7 @@ > /** @file > SRAT table parser >=20 > - 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 >=20 > @par Reference(s): > @@ -412,14 +412,16 @@ ParseAcpiSrat ( > return; > } >=20 > - // 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)'