From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: zhichao.gao@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Mon, 05 Aug 2019 21:58:24 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Aug 2019 21:58:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,352,1559545200"; d="scan'208";a="181870481" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 05 Aug 2019 21:58:23 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 21:58:22 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 5 Aug 2019 21:58:21 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.80]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.62]) with mapi id 14.03.0439.000; Tue, 6 Aug 2019 12:58:18 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "'sami.mujawar@arm.com'" , Krzysztof Koch CC: "Carsey, Jaben" , "Ni, Ray" , Matteo Carlini , nd 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//+plYCAAYNS8A== Date: Tue, 6 Aug 2019 04:58:17 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B81FD1D@SHSMSX101.ccr.corp.intel.com> References: <20190801084407.48712-1-krzysztof.koch@arm.com> <20190801084407.48712-3-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjljODE2NGYtZGI3ZS00NjQ3LTk1MWUtNWZmMTZhNDZlZWNhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiZWlrNERNQ0kxUHpCXC9NdWJxbEtoVnNPTFR1K2l1SnMwZmdDWFZ0aEErcm1UZHZ5Z1V3UjBVYUtCT1BcL1pRUm11In0= 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 OK, I got it. Now I have no comment of this patch. I think the description = of 'Length' should be 'the whole length of the GT block include GT Block Ti= mer Structure' or '20+("GT Block Timer Offset" - 20) + n * 40, where n ...'= in the ACPI spec. Reviewed-by: Zhichao Gao Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Sami Mujawar > Sent: Monday, August 5, 2019 5:55 PM > To: devel@edk2.groups.io; Gao, Zhichao ; Krzyszto= f > Koch > Cc: Carsey, Jaben ; Ni, Ray ; > Matteo Carlini ; nd > Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Preve= nt > buffer overruns >=20 > Hi Zhichao, >=20 > Please see my response inline. >=20 > Regards, >=20 > Sami Mujawar >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Gao, > Zhichao via Groups.Io > Sent: 05 August 2019 08:23 AM > To: devel@edk2.groups.io; Krzysztof Koch > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Preve= nt > buffer overruns >=20 > One confusion below: >=20 > > -----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 > > > > Modify the GTDT table parsing logic to prevent reading past the ACPI > > buffer lengths provided and to make it consistent with other table > > parsers. This includes converting the do-while loop in ParseAcpiGtdt()= into > a while loop. > > > > Remove a check which ensures that the entire Platform GT Block > > Structure buffer has been parsed. The ACPI specification does not ban > > from defining buffers which are larger than the size indicated by the > > count and sizes of substructures which constitute it. > > > > Change the data type of the Length parameter to the DumpGTBlock() > > function to reflect the width of the respective ACPI structure's field= . > > > > References: > > - ACPI 6.3, January 2019, Table 5-124 > > > > Signed-off-by: Krzysztof Koch > > --- > > > > Notes: > > v1: > > - Prevent buffer overruns in GTDT acpiview parser [Krzysztof] > > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c > > | 147 ++++++++++---------- > > 1 file changed, 76 insertions(+), 71 deletions(-) > > > > 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; > > > > /** > > @@ -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. > > > > - @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; > > > > - Offset =3D ParseAcpi ( > > - TRUE, > > - 2, > > - "GT Block", > > - Ptr, > > - Length, > > - PARSER_PARAMS (GtBlockParser) > > - ); > > - GTBlockTimerLength =3D (*GtBlockLength - Offset) / > > (*GtBlockTimerCount); > > - Length -=3D Offset; > > + ParseAcpi ( > > + TRUE, > > + 2, > > + "GT Block", > > + Ptr, > > + Length, > > + PARSER_PARAMS (GtBlockParser) > > + ); > > > > - 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; > > > > - 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) > > + ); >=20 > 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 > Block" > GT Block Timer Structure[] - Byte Offset: GT Block Timer Offset - "Array= of GT > Block Timer Structures. See Table 5-125" >=20 > Why the 'Byte Offset' is not 20? > 'Length - Offset' would be 'Length - 20' =3D=3D 'n * 40'. If the 'GT Blo= ck Timer > Offset' is not 20, its value should be lager 20. Then 'Length - Offset' = would > always less than 'n * 40'. It may miss some info of the GtBlockTimer. > Maybe all the platforms' GT block table's 'GT Block Timer Offset' is alw= ays 20. > If so, then there is no problem with this above section. >=20 > Did I misunderstand something? > [SAMI] The '20+n*40' in the 'Description' column in the ACPI Spec 6.3 Ta= ble 5- > 124 is an example of how the length field may be computed. > The 'GT Block Timer Offset' field could technically allow unused bytes > between the 'GT Block Timer Offset' field and the start of the first 'GT= Block > Timer Structure'. An OS is expected to read the 'GT Block Timer Offset' = field > to know where the 'GT Block Timer Structure' data starts. Conversely the > firmware must encode the length field appropriately. In this case the le= ngth > field would be '20+UnusedByteCount+(n*40)'. >=20 > The 'Length - Offset' is used to ensure we don't run past the buffer. Th= is also > reinforces the fact that ACPIview would be providing a view of the table= s as > an OS might see. >=20 > I will check if this can be made clearer in the specification. > [/SAMI] >=20 > Thanks, > Zhichao >=20 > > } > > } > > > > @@ -270,6 +256,7 @@ ParseAcpiGtdt ( > > ) > > { > > UINT32 Index; > > + UINT32 Offset; > > UINT8* TimerPtr; > > > > if (!Trace) { > > @@ -285,36 +272,54 @@ ParseAcpiGtdt ( > > PARSER_PARAMS (GtdtParser) > > ); > > > > - 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 >=20 >=20