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.151, mailfrom: zhichao.gao@intel.com) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by groups.io with SMTP; Sun, 21 Jul 2019 17:39:01 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Jul 2019 17:39:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,292,1559545200"; d="scan'208";a="188495096" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 21 Jul 2019 17:39:01 -0700 Received: from fmsmsx162.amr.corp.intel.com (10.18.125.71) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 21 Jul 2019 17:39:01 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by fmsmsx162.amr.corp.intel.com (10.18.125.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 21 Jul 2019 17:39:00 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.134]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.174]) with mapi id 14.03.0439.000; Mon, 22 Jul 2019 08:38:59 +0800 From: "Gao, Zhichao" To: Krzysztof Koch , "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: AQHVPWTRINuWpfgv2UykzyX5OuJLvKbRInsAgABqPXCAAAYwIIAAJbnQgAQXZGA= Date: Mon, 22 Jul 2019 00:38:59 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B8092ED@SHSMSX101.ccr.corp.intel.com> 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: 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 > -----Original Message----- > From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com] > Sent: Friday, July 19, 2019 6:41 PM > To: Gao, Zhichao ; devel@edk2.groups.io > Cc: nd > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer leng= th to > DumpGasStruct() >=20 > Hi Zhichao, >=20 > Sorry about private emails, I just mistakenly hit the wrong button. I for= got > that the emails do not get sent to the mailing list unless I hit 'Reply a= ll' (silly > mistake). >=20 > I see your point. I will submit v2 of this patchset with: > > DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); > Replaced with > > DumpGasStruct (DataPtr, 4, GAS_LENGTH); >=20 > Is that ok? Yes. Thanks for taking my advice. Thanks, Zhichao >=20 > Kind regards, >=20 > Krzysztof >=20 >=20 > -----Original Message----- > From: Gao, Zhichao > Sent: Friday, July 19, 2019 9:09 > To: Krzysztof Koch > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer leng= th to > DumpGasStruct() >=20 > HI Krzysztof, >=20 > Why do you send the email to me only? I think it is fine to send to the > community. It would let more people who are interest with this component > join the discuss. Maybe not . :( >=20 > Then for your comments: > Sorry, I missed the commit message before. But I don't think keeping an > inappropriate code with a commit massage to explain that is OK. If you ha= ve 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 w= hen > use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow. > DevInfoPtr DataPtr = EndDevInfoPtr > ---|---------------------------|------------------------------|---- > |<-- DbgDevInfoLen = -> | >=20 > Thanks, > Zhichao >=20 > > -----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 > > length to > > DumpGasStruct() > > > > Hi Zhichao, > > > > Please see my comments inline marked as [Krzysztof] > > > > Kind regards, > > > > Krzysztof > > > > -----Original Message----- > > From: Gao, Zhichao > > Sent: Friday, July 19, 2019 2:15 > > To: Krzysztof Koch ; devel@edk2.groups.io > > Cc: Carsey, Jaben ; Ni, Ray > > ; Sami Mujawar ; Matteo > > Carlini ; nd > > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > length to > > DumpGasStruct() > > > > > > > > > -----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 > > > ; Gao, Zhichao ; > > > Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com > > > Subject: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > > length to > > > DumpGasStruct() > > > > > > Modify the signature of the DumpGasStruct() function to include the > > > buffer length parameter and to return the number of bytes parsed by > > > the > > function. > > > > > > This way it becomes possible to prevent buffer overruns when dumping > > > Generic Address Structure's (GAS) fields in the acpiview table parser= s. > > > > > > Update all existing DumpGasStruct() calls in acpiview to add the > > > 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 > > > 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 > > > 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); > > > > This input length should be GAS_LENGTH. *DbgDevInfoLen is the length > > of the whole structure and the DataPtr is increased during the loop. > > Inputing such a length would give the ParseAcpi function a chance to > > overrun the DataPtr. > > > > Thanks, > > Zhichao > > > > [Krzysztof] You're right. This patch does not eliminate the > > possibility for a buffer overrun when parsing the GAS structure. This > > patch series does not implement that yet, it's merely a refactoring > > change in preparation for the patches which actually deal with the > > problem. The patches which prevent buffer overruns in acpiview parsers > > are pending on the patches which are currently in review. > > > > In the commit message I state that the change of the DumpGasStruct() > > function signature makes it "possible to prevent buffer overruns when > > dumping Generic Address Structure's (GAS) fields in the acpiview table > > parsers." The validation is coming soon. > > > > > PrintFieldName (4, L"Address Size"); > > > Print (L"0x%x\n", AddrSize[Index]); > > > DataPtr +=3D GAS_LENGTH; > > > -- > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > > recipient, please notify the sender immediately and do not disclose > > the contents to any other person, use it for any purpose, or store or > > copy the information in any medium. Thank you.