From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: zhichao.gao@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Mon, 05 Aug 2019 00:23:05 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Aug 2019 00:23:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,349,1559545200"; d="scan'208";a="176232071" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 05 Aug 2019 00:23:04 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 00:23:03 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 00:23:03 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.80]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.163]) with mapi id 14.03.0439.000; Mon, 5 Aug 2019 15:23:01 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Carsey, Jaben" , "Ni, Ray" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns Thread-Topic: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent buffer overruns Thread-Index: AQHVSEVvm26yzw8Q7kOmQV3q3GmfZabsJ2BQ Date: Mon, 5 Aug 2019 07:23:01 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.intel.com> References: <20190801084407.48712-1-krzysztof.koch@arm.com> <20190801084407.48712-3-krzysztof.koch@arm.com> In-Reply-To: <20190801084407.48712-3-krzysztof.koch@arm.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] 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 One confusion below: > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Thursday, August 1, 2019 4:44 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray ; > Gao, Zhichao ; Sami.Mujawar@arm.com; > Matteo.Carlini@arm.com; nd@arm.com > Subject: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent > buffer overruns >=20 > Modify the GTDT table parsing logic to prevent reading past the ACPI buf= fer > lengths provided and to make it consistent with other table parsers. Thi= s > includes converting the do-while loop in ParseAcpiGtdt() into a while lo= op. >=20 > Remove a check which ensures that the entire Platform GT Block Structure > buffer has been parsed. The ACPI specification does not ban from definin= g > buffers which are larger than the size indicated by the count and sizes = of > substructures which constitute it. >=20 > Change the data type of the Length parameter to the DumpGTBlock() > function to reflect the width of the respective ACPI structure's field. >=20 > References: > - ACPI 6.3, January 2019, Table 5-124 >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Notes: > v1: > - Prevent buffer overruns in GTDT acpiview parser [Krzysztof] >=20 > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c > | 147 ++++++++++---------- > 1 file changed, 76 insertions(+), 71 deletions(-) >=20 > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > .c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > .c > index > 1e5b5764f50a2d29aa904c889bc89af5bdc3af5c..57174e14c80072f12b90e1996e > be8f0002d0c404 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser > .c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars > +++ er.c > @@ -23,7 +23,6 @@ STATIC CONST UINT8* PlatformTimerType; STATIC > CONST UINT16* PlatformTimerLength; STATIC CONST UINT32* > GtBlockTimerCount; STATIC CONST UINT32* GtBlockTimerOffset; -STATIC > CONST UINT16* GtBlockLength; STATIC ACPI_DESCRIPTION_HEADER_INFO > AcpiHdrInfo; >=20 > /** > @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER > GtPlatformTimerHeaderParser[] =3D { **/ STATIC CONST ACPI_PARSER > GtBlockParser[] =3D { > {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL}, > - {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL}, > + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL}, > {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL}, > {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, = NULL}, > {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount, @@ - > 168,56 +167,43 @@ STATIC CONST ACPI_PARSER > SBSAGenericWatchdogParser[] =3D { > /** > This function parses the Platform GT Block. >=20 > - @param [in] Ptr Pointer to the start of the GT Block data. > - @param [in] Length Length of the GT Block structure. > + @param [in] Ptr Pointer to the start of the GT Block data. > + @param [in] Length Length of the GT Block structure. > **/ > STATIC > VOID > DumpGTBlock ( > IN UINT8* Ptr, > - IN UINT32 Length > + IN UINT16 Length > ) > { > UINT32 Index; > UINT32 Offset; > - UINT32 GTBlockTimerLength; >=20 > - Offset =3D ParseAcpi ( > - TRUE, > - 2, > - "GT Block", > - Ptr, > - Length, > - PARSER_PARAMS (GtBlockParser) > - ); > - GTBlockTimerLength =3D (*GtBlockLength - Offset) / (*GtBlockTimerCoun= t); > - Length -=3D Offset; > + ParseAcpi ( > + TRUE, > + 2, > + "GT Block", > + Ptr, > + Length, > + PARSER_PARAMS (GtBlockParser) > + ); >=20 > - if (*GtBlockTimerCount !=3D 0) { > - Ptr +=3D (*GtBlockTimerOffset); > - Index =3D 0; > - while ((Index < (*GtBlockTimerCount)) && (Length >=3D > GTBlockTimerLength)) { > - Offset =3D ParseAcpi ( > - TRUE, > - 2, > - "GT Block Timer", > - Ptr, > - GTBlockTimerLength, > - PARSER_PARAMS (GtBlockTimerParser) > - ); > - // Increment by GT Block Timer structure size > - Ptr +=3D Offset; > - Length -=3D Offset; > - Index++; > - } > + Offset =3D *GtBlockTimerOffset; > + Index =3D 0; >=20 > - if (Length !=3D 0) { > - IncrementErrorCount (); > - Print ( > - L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n", > - Length > - ); > - } > + // Parse the specified number of GT Block Timer Structures or the GT > + Block // Structure buffer length. Whichever is minimum. > + while ((Index++ < *GtBlockTimerCount) && > + (Offset < Length)) { > + Offset +=3D ParseAcpi ( > + TRUE, > + 2, > + "GT Block Timer", > + Ptr + Offset, > + Length - Offset, > + PARSER_PARAMS (GtBlockTimerParser) > + ); The above confuse me a lot. ACPI Spec 6.3 Table 5-124: Length - "20+n*40, where n is the number of timers implemented in the GT B= lock" GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array o= f GT Block Timer Structures. See Table 5-125" Why the 'Byte Offset' is not 20? 'Length - Offset' would be 'Length - 20' =3D=3D 'n * 40'. If the 'GT Block= Timer Offset' is not 20, its value should be lager 20. Then 'Length - Offs= et' would always less than 'n * 40'. It may miss some info of the GtBlockT= imer. Maybe all the platforms' GT block table's 'GT Block Timer Offset' is alway= s 20. If so, then there is no problem with this above section. Did I misunderstand something? Thanks, Zhichao > } > } >=20 > @@ -270,6 +256,7 @@ ParseAcpiGtdt ( > ) > { > UINT32 Index; > + UINT32 Offset; > UINT8* TimerPtr; >=20 > if (!Trace) { > @@ -285,36 +272,54 @@ ParseAcpiGtdt ( > PARSER_PARAMS (GtdtParser) > ); >=20 > - if (*GtdtPlatformTimerCount !=3D 0) { > - TimerPtr =3D Ptr + (*GtdtPlatformTimerOffset); > - Index =3D 0; > - do { > - // Parse the Platform Timer Header > - ParseAcpi ( > - FALSE, > - 0, > - NULL, > - TimerPtr, > - 4, // GT Platform Timer structure header length. > - PARSER_PARAMS (GtPlatformTimerHeaderParser) > + TimerPtr =3D Ptr + *GtdtPlatformTimerOffset; Offset =3D > + *GtdtPlatformTimerOffset; Index =3D 0; > + > + // Parse the specified number of Platform Timer Structures or the > + GTDT // buffer length. Whichever is minimum. > + while ((Index++ < *GtdtPlatformTimerCount) && > + (Offset < AcpiTableLength)) { > + // Parse the Platform Timer Header to obtain Length and Type > + ParseAcpi ( > + FALSE, > + 0, > + NULL, > + TimerPtr, > + AcpiTableLength - Offset, > + PARSER_PARAMS (GtPlatformTimerHeaderParser) > + ); > + > + // Make sure the Platform Timer is inside the table. > + if ((Offset + *PlatformTimerLength) > AcpiTableLength) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid Platform Timer Structure length. " \ > + L"PlatformTimerLength =3D %d. RemainingTableBufferLength =3D = %d. " \ > + L"GTDT parsing aborted.\n", > + *PlatformTimerLength, > + AcpiTableLength - Offset > ); > - switch (*PlatformTimerType) { > - case EFI_ACPI_6_2_GTDT_GT_BLOCK: > - DumpGTBlock (TimerPtr, *PlatformTimerLength); > - break; > - case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG: > - DumpWatchdogTimer (TimerPtr, *PlatformTimerLength); > - break; > - default: > - IncrementErrorCount (); > - Print ( > - L"ERROR: INVALID Platform Timer Type =3D %d\n", > - *PlatformTimerType > - ); > - break; > - } // switch > - TimerPtr +=3D (*PlatformTimerLength); > - Index++; > - } while (Index < *GtdtPlatformTimerCount); > - } > + return; > + } > + > + switch (*PlatformTimerType) { > + case EFI_ACPI_6_3_GTDT_GT_BLOCK: > + DumpGTBlock (TimerPtr, *PlatformTimerLength); > + break; > + case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG: > + DumpWatchdogTimer (TimerPtr, *PlatformTimerLength); > + break; > + default: > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid Platform Timer Type =3D %d\n", > + *PlatformTimerType > + ); > + break; > + } // switch > + > + TimerPtr +=3D *PlatformTimerLength; > + Offset +=3D *PlatformTimerLength; > + } // while > } > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >=20 >=20 >=20 >=20