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=ocvMbsxP; spf=pass (domain: arm.com, ip: 40.107.7.71, mailfrom: krzysztof.koch@arm.com) Received: from EUR04-HE1-obe.outbound.protection.outlook.com (EUR04-HE1-obe.outbound.protection.outlook.com [40.107.7.71]) by groups.io with SMTP; Fri, 19 Jul 2019 03:40:49 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AaZAdjtYkYmTh55Ig3XKpZnGNjl2b5Zs6hxY+R9dEHOa7vTiG65d0Ogta++FDN44q9jpNuWDPStwaayn9xpU1fdhniT+VPrzUyhXiA/R4hfTlgm3HrtSFVptL2ZGBxbjvQPrkcMUM9hl2jz4CTPh/hFLdv9uVcMTJxmL6VRS0updVbqEz6ZWU289tIH8SUXnIRsQhzpiRdPH4Pc60ICrN3+RwJbmwjVAhb0v48gFIMsKe+CdUvEsAF43CUrBS/BaKuKTW2CeIhCYMpAH7hPbN5Av1QYr/Qvor9sPkuc2GerC4sokYELRJfkbgEHAuOpHgeP/SF2H1t2ByhhwhK/91w== 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=FlADgEQCQXUJFpfLhc8msr33zp+4MiC0rmGLeEnr7ms=; b=Twrk1xVzUFyiHCvnG229OW5rWngitOMiAUvrabLKXZ+SL3YL/R/6T8LvaqsrdJsT0PSnWW0lOjc0dwsIWJzPOiv3m3aFjtsE3E5tcbTCeb+ltySLEbY0K2gUTWNfhks7MJVlyLW62T7DTHxeCKAJm04OTNc58z+IFRjck2oasGdLIcivw4iu1FzEAfzSyuzI+qKyH+GgHnorgP2n2bVOhOQ8PnWUv5T8GvVeqrnZLClxCLlKHM30bsij4k6fQHT4Sa2CCbAIWM8JCgI4oxusquKtHqSyrttoQ3c1Kq15gu6G7cYRqZOQRLowT7DZHsybqZI7PdTpWFA7Vx6oxFCAag== 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=FlADgEQCQXUJFpfLhc8msr33zp+4MiC0rmGLeEnr7ms=; b=ocvMbsxPC8G9q/t80ZTGK6oSJaQUxuJmSBu1GQ17MFCICiUEJcmfQ/uGitDsrNSlEPwd3WJzndT15NHibg50tTXPTsRydrhD9VC21xMp4Pv733IzN/CbNTFlhqE0iww+3A7yVhNg1AJQKYOgST7++auWs7umFfmPaRAL+QWIX4s= Received: from VE1PR08MB4783.eurprd08.prod.outlook.com (10.255.114.16) by VE1PR08MB5118.eurprd08.prod.outlook.com (20.179.30.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2073.14; Fri, 19 Jul 2019 10:40:45 +0000 Received: from VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::c144:7735:6511:6492]) by VE1PR08MB4783.eurprd08.prod.outlook.com ([fe80::c144:7735:6511:6492%3]) with mapi id 15.20.2094.013; Fri, 19 Jul 2019 10:40:45 +0000 From: "Krzysztof Koch" To: "Gao, Zhichao" , "devel@edk2.groups.io" CC: nd Subject: Re: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct() Thread-Topic: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct() Thread-Index: AQHVPWTRINuWpfgv2UykzyX5OuJLvKbRInsAgABqPXCAAAYwIIAAJbnQ Date: Fri, 19 Jul 2019 10:40:45 +0000 Message-ID: References: <20190718123142.5696-1-krzysztof.koch@arm.com> <20190718123142.5696-2-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B808DB0@SHSMSX101.ccr.corp.intel.com> <3CE959C139B4C44DBEA1810E3AA6F9000B808FFA@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B808FFA@SHSMSX101.ccr.corp.intel.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: cafc2bd9-6c82-4aba-9214-f5de2f5a90c9.1 x-checkrecipientchecked: true authentication-results: spf=none (sender IP is ) smtp.mailfrom=Krzysztof.Koch@arm.com; x-originating-ip: [217.140.106.53] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: b1af845f-8bf5-46fd-2540-08d70c358e29 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:VE1PR08MB5118; x-ms-traffictypediagnostic: VE1PR08MB5118: x-microsoft-antispam-prvs: nodisclaimer: True x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 01039C93E4 x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(39860400002)(396003)(376002)(346002)(136003)(366004)(13464003)(199004)(189003)(25786009)(26005)(186003)(7696005)(5024004)(3846002)(6116002)(2906002)(14444005)(8676002)(446003)(476003)(11346002)(229853002)(6506007)(53546011)(76176011)(6436002)(33656002)(71190400001)(74316002)(14454004)(99286004)(9686003)(53936002)(486006)(55016002)(102836004)(81156014)(7736002)(81166006)(71200400001)(2501003)(8936002)(478600001)(6246003)(66476007)(66946007)(66556008)(76116006)(316002)(66446008)(64756008)(110136005)(66066001)(52536014)(68736007)(256004)(305945005)(5660300002)(4326008)(86362001);DIR:OUT;SFP:1101;SCL:1;SRVR:VE1PR08MB5118;H:VE1PR08MB4783.eurprd08.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX: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: qAjHSTOi9N8pFmclX6FfAowQ5cvIeNB22Q0Q4yBs40bZUroG9oJRwoKOpJjS7d9yk9brWv4Mpby9ZGfBbbc6ETs7/1NYExrCV56tbYf13mhX3/yE2vIm+zJWkR65NkYj0gIEkZ54+deiS/0ZPuXFeG0l7mT6RhUN+dkuI89YMEB5dlYbeSirsTAXoL8GQ3W0aT1ToTftc/tzIDIDOQ2yKINVCwa+Sv6mz3/OY6R1vOZLJspT9GojNWU2hO2VYojbLOt07G+Iez4Da9q5pXTj69CL3Pg0v66ZkqDidbdsndbDMNECsCRvEb+ZixKWaCfiE3lIv5+egualLZlYoiavm40HgHZCeS72IYLHx7U7vjcOg+Iafi9Nn0a+OWZ39tIXPg3uGa9/peVoaJbavOTbUFIv3PmEtGNYTicwxMfAU00= MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: b1af845f-8bf5-46fd-2540-08d70c358e29 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2019 10:40:45.2916 (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: Krzysztof.Koch@arm.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR08MB5118 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zhichao, Sorry about private emails, I just mistakenly hit the wrong button. I forgo= t that the emails do not get sent to the mailing list unless I hit 'Reply a= ll' (silly mistake). I see your point. I will submit v2 of this patchset with: > DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); Replaced with > DumpGasStruct (DataPtr, 4, GAS_LENGTH); Is that ok? Kind regards, Krzysztof -----Original Message----- From: Gao, Zhichao =20 Sent: Friday, July 19, 2019 9:09 To: Krzysztof Koch Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length= to DumpGasStruct() HI Krzysztof, Why do you send the email to me only? I think it is fine to send to the com= munity. It would let more people who are interest with this component join = the discuss. Maybe not . :( Then for your comments: Sorry, I missed the commit message before. But I don't think keeping an in= appropriate code with a commit massage to explain that is OK. If you have a= plan to fix that, it should combine into one patch. Back to the code, I don't find out the possibility of overrun of buffer whe= n use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow. DevInfoPtr DataPtr = EndDevInfoPtr ---|---------------------------|------------------------------|---- |<-- DbgDevInfoLen = -> | Thanks, Zhichao > -----Original Message----- > From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com] > Sent: Friday, July 19, 2019 3:39 PM > To: Gao, Zhichao > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer=20 > length to > DumpGasStruct() >=20 > Hi Zhichao, >=20 > Please see my comments inline marked as [Krzysztof] >=20 > Kind regards, >=20 > Krzysztof >=20 > -----Original Message----- > From: Gao, Zhichao > Sent: Friday, July 19, 2019 2:15 > To: Krzysztof Koch ; devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray=20 > ; Sami Mujawar ; Matteo=20 > Carlini ; nd > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer=20 > length to > DumpGasStruct() >=20 >=20 >=20 > > -----Original Message----- > > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com] > > Sent: Thursday, July 18, 2019 8:32 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: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer=20 > > length to > > DumpGasStruct() > > > > Modify the signature of the DumpGasStruct() function to include the=20 > > buffer length parameter and to return the number of bytes parsed by=20 > > the > function. > > > > This way it becomes possible to prevent buffer overruns when dumping=20 > > Generic Address Structure's (GAS) fields in the acpiview table parsers. > > > > Update all existing DumpGasStruct() calls in acpiview to add the=20 > > length argument. > > > > Signed-off-by: Krzysztof Koch > > --- > > > > Notes: > > v1: > > - Modify DumpGasStruct() signature [Krzysztof] > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c = | 26 > > +++++++++++--------- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h = | 8 > > ++++-- > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > > | 2 +- > > 3 files changed, 22 insertions(+), 14 deletions(-) > > > > diff --git=20 > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > index > > > 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3 > > db89332197c0dc0e 100644 > > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] =3D { > > > > @param [in] Ptr Pointer to the start of the buffer. > > @param [in] Indent Number of spaces to indent the output. > > + @param [in] Length Length of the GAS structure buffer. > > + > > + @retval Number of bytes parsed. > > **/ > > -VOID > > +UINT32 > > EFIAPI > > DumpGasStruct ( > > IN UINT8* Ptr, > > - IN UINT32 Indent > > + IN UINT32 Indent, > > + IN UINT32 Length > > ) > > { > > Print (L"\n"); > > - ParseAcpi ( > > - TRUE, > > - Indent, > > - NULL, > > - Ptr, > > - GAS_LENGTH, > > - PARSER_PARAMS (GasParser) > > - ); > > + return ParseAcpi ( > > + TRUE, > > + Indent, > > + NULL, > > + Ptr, > > + Length, > > + PARSER_PARAMS (GasParser) > > + ); > > } > > > > /** > > @@ -621,7 +625,7 @@ DumpGas ( > > IN UINT8* Ptr > > ) > > { > > - DumpGasStruct (Ptr, 2); > > + DumpGasStruct (Ptr, 2, GAS_LENGTH); > > } > > > > /** > > diff --git=20 > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > index > > > 7657892d9fd2e2e14c6578611ff0cf1b6f6cd750..20ca358bddfa5953bfb1d1beba > > ebbf3079eaba01 100644 > > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > @@ -405,12 +405,16 @@ ParseAcpi ( > > > > @param [in] Ptr Pointer to the start of the buffer. > > @param [in] Indent Number of spaces to indent the output. > > + @param [in] Length Length of the GAS structure buffer. > > + > > + @retval Number of bytes parsed. > > **/ > > -VOID > > +UINT32 > > EFIAPI > > DumpGasStruct ( > > IN UINT8* Ptr, > > - IN UINT32 Indent > > + IN UINT32 Indent, > > + IN UINT32 Length > > ); > > > > /** > > diff --git > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > r.c > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > r.c > > index > > > 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2c47a3f848aa2dd512c53343e > > cf1c3c285173dd6 100644 > > --- > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > r.c > > +++ > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars > > +++ er.c > > @@ -164,7 +164,7 @@ DumpDbgDeviceInfo ( > > AddrSize =3D (UINT32*)(Ptr + (*AddrSizeOffset)); > > while (Index < (*GasCount)) { > > PrintFieldName (4, L"BaseAddressRegister"); > > - DumpGasStruct (DataPtr, 4); > > + DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); >=20 > This input length should be GAS_LENGTH. *DbgDevInfoLen is the length=20 > of the whole structure and the DataPtr is increased during the loop.=20 > Inputing such a length would give the ParseAcpi function a chance to=20 > overrun the DataPtr. >=20 > Thanks, > Zhichao >=20 > [Krzysztof] You're right. This patch does not eliminate the=20 > possibility for a buffer overrun when parsing the GAS structure. This=20 > patch series does not implement that yet, it's merely a refactoring=20 > change in preparation for the patches which actually deal with the=20 > problem. The patches which prevent buffer overruns in acpiview parsers=20 > are pending on the patches which are currently in review. >=20 > In the commit message I state that the change of the DumpGasStruct()=20 > function signature makes it "possible to prevent buffer overruns when=20 > dumping Generic Address Structure's (GAS) fields in the acpiview table=20 > parsers." The validation is coming soon. >=20 > > PrintFieldName (4, L"Address Size"); > > Print (L"0x%x\n", AddrSize[Index]); > > DataPtr +=3D GAS_LENGTH; > > -- > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > >=20 > IMPORTANT NOTICE: The contents of this email and any attachments are=20 > confidential and may also be privileged. If you are not the intended=20 > recipient, please notify the sender immediately and do not disclose=20 > the contents to any other person, use it for any purpose, or store or=20 > copy the information in any medium. Thank you.