From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@armh.onmicrosoft.com header.s=selector2-armh-onmicrosoft-com header.b=5UB0CEVt; spf=pass (domain: arm.com, ip: 40.107.4.40, mailfrom: sami.mujawar@arm.com) Received: from EUR03-DB5-obe.outbound.protection.outlook.com (EUR03-DB5-obe.outbound.protection.outlook.com [40.107.4.40]) by groups.io with SMTP; Mon, 05 Aug 2019 02:54:47 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CBkaQ1B9AnM4vD0xFEpp90egdAa9VQQTRfb4F9GAKIgu4OH2DNM2tHirrl9Ogl+irWSKCLLfTyfHfLKYf2cXkvtxph7QDg+lgd48PLI/yRHYdt7ELNPY5cF26CbECM5GkB1fPGlbGo4J0hWtBD30DdUrTCbFQH8eO7OlxvBAqikY98YZ7O0p8kmnpIvelcMP+yOzIx8mFOKhL/rpCrQtcIbtQao1/KF7IyJFKLmj6x5vSAgsZM3fBHE3USgbuEnJPh2gmWXAnp+3nVZdaGP2486eIKeBgHa5pFksu1LNsjdkbmx2asIU0kooPda2sfZ5q7fzIllRVldfr/wwum9/3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Nex24H5f298hQi2sDmgGNzMJ6PUyHZE0HeV5VmstMEA=; b=dho71blp27hStFNj98Ttvdxi+ztPba+epTcQDpINdFfdda58EByv9+QYbqtRx7C2cLKuwkoJNyyQ1hiEeKckiO/7jEAarmILWukUBwHpU9dYEeoeWcER77Z4jwRlA81D1ErJZf3CFiysEwLZX77U44OZsxEWkjtEPirMUPR34F0rJ3pm6Ko6eBNCNqg09S6qRWP2HdDa1+Ysg9SJinjg1jUMVGqJgEcSRSHotbqXXLbEa2xzAJ77d/6J0n0EnJn02f0OXtCQ9F5zJ7CQBxMTjgdMfKmuG2N8uZKIMfFuZxGMPy5AQFC79tqV4YQcj+ctuet45Lvc8SDgj2i3vim6oQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1;spf=pass smtp.mailfrom=arm.com;dmarc=pass action=none header.from=arm.com;dkim=pass header.d=arm.com;arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Nex24H5f298hQi2sDmgGNzMJ6PUyHZE0HeV5VmstMEA=; b=5UB0CEVtKYK0swidj8PcIzq9q0gpU4MLHaLbqr2igOsUZIfFAFssAmf/XLigficWG2DMrrvJFIuWTgbrypd4v8ufZRVj+st4yn9pe8y0/b42Oz5HZuMshiTRnm4r71IMGsJ85chSgKBNTrxeGR7GLARBYdU0OvEzZSdWkBUyMbw= Received: from DB6PR0802MB2375.eurprd08.prod.outlook.com (10.172.228.142) by DB6PR0802MB2248.eurprd08.prod.outlook.com (10.172.227.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2136.14; Mon, 5 Aug 2019 09:54:43 +0000 Received: from DB6PR0802MB2375.eurprd08.prod.outlook.com ([fe80::88a4:74c4:c4b7:aa1a]) by DB6PR0802MB2375.eurprd08.prod.outlook.com ([fe80::88a4:74c4:c4b7:aa1a%5]) with mapi id 15.20.2136.018; Mon, 5 Aug 2019 09:54:43 +0000 From: "Sami Mujawar" To: "devel@edk2.groups.io" , "zhichao.gao@intel.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: AQHVSEVim2ZATp65Q06JbS7L1iaaLabsLK+AgAAePAA= Date: Mon, 5 Aug 2019 09:54:43 +0000 Message-ID: References: <20190801084407.48712-1-krzysztof.koch@arm.com> <20190801084407.48712-3-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B81FAE0@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: 05fd4ded-39bc-4ebd-b79f-06ce3efb6bcb.2 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Sami.Mujawar@arm.com; x-originating-ip: [217.140.106.50] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1bf7173e-7e20-49c3-74f8-08d7198af0f8 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600148)(711020)(4605104)(1401327)(4618075)(2017052603328)(7193020);SRVR:DB6PR0802MB2248; x-ms-traffictypediagnostic: DB6PR0802MB2248: x-ms-exchange-purlcount: 3 x-microsoft-antispam-prvs: nodisclaimer: True x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 01208B1E18 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(376002)(136003)(346002)(366004)(39860400002)(396003)(189003)(199004)(13464003)(6636002)(6306002)(9686003)(229853002)(55016002)(74316002)(99286004)(6116002)(33656002)(2906002)(53936002)(3846002)(7736002)(4326008)(6246003)(26005)(102836004)(53546011)(71190400001)(71200400001)(6506007)(7696005)(966005)(8936002)(6436002)(316002)(54906003)(5660300002)(110136005)(186003)(14454004)(256004)(8676002)(81156014)(81166006)(25786009)(66066001)(76176011)(486006)(446003)(305945005)(478600001)(68736007)(476003)(2501003)(76116006)(11346002)(86362001)(52536014)(66446008)(66946007)(66556008)(66476007)(64756008);DIR:OUT;SFP:1101;SCL:1;SRVR:DB6PR0802MB2248;H:DB6PR0802MB2375.eurprd08.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: z/nZWLAAFeUHH4jLjrbkj/9TJwNRuSOr9DmT26hRyEm33MdZ1vo/b9Yu2tdcPFi+76JD0UksuPqCN5QIGTAzZgWbo0CInoiAr0NpiiZGNbDFvYlm10fQK4P0yWp4LNOvDypk6j7N6QAP4vYxROfbR8frhWUxTYxb9X5fVyK+WHtBUQgGpsoWMa7FMiDlUej4qEEVtxWZsvGp7oTEbLm/nrMcNAq4DULczr1cB1Z4WBBuELMypvugiqMGoAYwI3c3lgK4eF7Ntjx8LVQt93buKGSbyjbNTAErKhA571PNcsBx87RZF7oYhIfKTZ4YIQDjcN0h+qT0rMkmFhpFF1yas85yBA8mcWoeH2kfgm6NMYtx5PmLkBInen1cgnGfIiqKOLBqOZdvMPYjyC9A2HF4PC2cU1/qSQhwO3/y+72hG80= MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1bf7173e-7e20-49c3-74f8-08d7198af0f8 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Aug 2019 09:54:43.3971 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Sami.Mujawar@arm.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0802MB2248 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zhichao, Please see my response inline. Regards, Sami Mujawar -----Original Message----- From: devel@edk2.groups.io On Behalf Of Gao, Zhicha= o via Groups.Io Sent: 05 August 2019 08:23 AM To: devel@edk2.groups.io; Krzysztof Koch Cc: Carsey, Jaben ; Ni, Ray ; Sa= mi Mujawar ; Matteo Carlini ;= nd Subject: Re: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent= buffer overruns One confusion below: > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of=20 > Krzysztof Koch > Sent: Thursday, August 1, 2019 4:44 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray=20 > ; Gao, Zhichao ;=20 > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > Subject: [edk2-devel] [PATCH v1 2/6] ShellPkg: acpiview: GTDT: Prevent= =20 > buffer overruns >=20 > Modify the GTDT table parsing logic to prevent reading past the ACPI=20 > buffer lengths provided and to make it consistent with other table=20 > parsers. This includes converting the do-while loop in ParseAcpiGtdt() i= nto a while loop. >=20 > Remove a check which ensures that the entire Platform GT Block=20 > Structure buffer has been parsed. The ACPI specification does not ban=20 > from defining buffers which are larger than the size indicated by the=20 > count and sizes of substructures which constitute it. >=20 > Change the data type of the Length parameter to the DumpGTBlock()=20 > 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=20 > CONST UINT16* PlatformTimerLength; STATIC CONST UINT32*=20 > GtBlockTimerCount; STATIC CONST UINT32* GtBlockTimerOffset; -STATIC=20 > CONST UINT16* GtBlockLength; STATIC ACPI_DESCRIPTION_HEADER_INFO=20 > AcpiHdrInfo; >=20 > /** > @@ -127,7 +126,7 @@ STATIC CONST ACPI_PARSER=20 > GtPlatformTimerHeaderParser[] =3D { **/ STATIC CONST ACPI_PARSER=20 > 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, @@= =20 > - > 168,56 +167,43 @@ STATIC CONST ACPI_PARSER SBSAGenericWatchdogParser[]= =20 > =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) /=20 > (*GtBlockTimerCount); > - 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=20 > + 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? [SAMI] The '20+n*40' in the 'Description' column in the ACPI Spec 6.3 Tabl= e 5-124 is an example of how the length field may be computed.=20 The 'GT Block Timer Offset' field could technically allow unused bytes bet= ween 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' fi= eld to know where the 'GT Block Timer Structure' data starts. Conversely th= e firmware must encode the length field appropriately. In this case the len= gth field would be '20+UnusedByteCount+(n*40)'. The 'Length - Offset' is used to ensure we don't run past the buffer. This= also reinforces the fact that ACPIview would be providing a view of the ta= bles as an OS might see. I will check if this can be made clearer in the specification. [/SAMI] 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=20 > + *GtdtPlatformTimerOffset; Index =3D 0; > + > + // Parse the specified number of Platform Timer Structures or the=20 > + 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