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; Tue, 06 Aug 2019 18:52:19 -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 orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Aug 2019 18:52:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,353,1559545200"; d="scan'208";a="192794608" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 06 Aug 2019 18:52:18 -0700 Received: from fmsmsx609.amr.corp.intel.com (10.18.126.89) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 6 Aug 2019 18:52:18 -0700 Received: from fmsmsx609.amr.corp.intel.com (10.18.126.89) by fmsmsx609.amr.corp.intel.com (10.18.126.89) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 6 Aug 2019 18:52:17 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by fmsmsx609.amr.corp.intel.com (10.18.126.89) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 6 Aug 2019 18:52:17 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.80]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.204]) with mapi id 14.03.0439.000; Wed, 7 Aug 2019 09:52:15 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Carsey, Jaben" , "Ni, Ray" , Sami Mujawar , Matteo Carlini , nd Subject: Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Thread-Topic: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Thread-Index: AQHVSEVpsSE81gr5hk6xm4Bgvoopw6bsHvjg//+X1oCAAgg2wP//skkAgAFzPjA= Date: Wed, 7 Aug 2019 01:52:14 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B820910@SHSMSX101.ccr.corp.intel.com> References: <20190801084407.48712-1-krzysztof.koch@arm.com> <20190801084407.48712-2-krzysztof.koch@arm.com> <3CE959C139B4C44DBEA1810E3AA6F9000B81EA99@SHSMSX101.ccr.corp.intel.com> <3CE959C139B4C44DBEA1810E3AA6F9000B81FE56@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 OK. I got your point.=20 1. The length of debug device info structure indicate the correct length s= hould be parsed. I'd prefer to see this as a backward compatibility as you = mentioned. See below: The ACPI table may be updated to extend some of the table. For example, if= DBG2->Debug Device Info structure is enlarged (add new field after Address= SizeOffset), we would update the DbgDevInforParser and that would cause an = incorrect parsing of old version Debug device info structure. So it is requ= ired to get the length of the structure first. I am not sure if it is requi= red for the acpi table header. 2. As you mentioned, using length to parse a structure may cause some vari= able not updated. I didn't consider that before. That's good point. If it i= s not updated, it would use its default value or the previous value that i= nitialize thru the previous ParseAcpi. So I think it is required to get par= sed value (return value of ParseAcpi) to judge if the variable is valid. For this patch, I have no comments. Reviewed-by: Zhichao Gao Thanks, Zhichao > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Tuesday, August 6, 2019 6:45 PM > To: devel@edk2.groups.io; Gao, Zhichao > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Preve= nt > buffer overruns >=20 > Hi Zhichao, >=20 > Thanks for the feedback. I had a look at your code and I have put commen= ts > inline (in GitHub) to describe why I think it does not achieve the same > functionality as the patch I've submitted. >=20 > I copied my comments below as well, so that they're easier to access: >=20 > """ > The ParseAcpi() call (Line 113) will parse either the entire DbgDevInfoP= arser[] > array or as much data as there is left in the ACPI table buffer. I agree= this > prevents buffer overruns with respect to the ACPI table buffer. However, > the parser now ignores the length of the Debug Device Information Struct= ure > (loaded into the *DbgDevInfoLen variable) when dumping its contents. >=20 > Here is an example: > If the DBG2 table buffer is 100-byte long, and the Debug Device Informat= ion > Structure is (let's say) located at offset 20 with a byte-size (as descr= ibed in > the 'Length' field) of only 10 bytes, then we have a problem. >=20 > The DbgDevInfoParser[] array says that 22 bytes should be parsed, howeve= r, > the user-provided structure length is 10. I believe that only 10 bytes s= hould > be parsed to reflect what an OS would do in this situation. >=20 > This is why I created a new ACPI_PARSER array in my submitted patch to: > 1. Read the Length of the Debug Device Information Structure 2. Validate= the > Length against the length of the DBG2 table buffer 3. Use the Length to > control how many statements from DbgDevInfoParser[] should be executed. >=20 > If we print only as much data as the ACPI table writer has specified the= n any > errors in the 'Length' field are easier to detect. You can easily see th= at some > data is missing and this is due to the 'Length' field having wrong value= . >=20 > Reading the 'Length' field before the whole structure is dumped is also > important for our acpiview implementation for the sake of backward > compatibility. As ACPI tables usually get updated by appending new field= s to > existing structures. If someone provides us with a Length that matches t= he > old DBG2 version then we won't print the fields that got recently added = to > DbgDevInfoParser[] due to a spec update. >=20 > I understand there is still an issue of some variables not getting updat= ed > correctly because we haven't parsed enough of the DbgDevInfoParser[], fo= r > example, the AddrSizeOffset variable. But my next patch series adds code= to > detect NULL pointers in all parsers. > """ >=20 > Please let me know what you think. >=20 > Kind regards, >=20 > Krzysztof >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Gao, > Zhichao via Groups.Io > Sent: Tuesday, August 6, 2019 8:43 > To: devel@edk2.groups.io; Krzysztof Koch > Cc: Carsey, Jaben ; Ni, Ray ; > Sami Mujawar ; Matteo Carlini > ; nd > Subject: Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Preve= nt > buffer overruns >=20 > I got your point. How about this: > https://github.com/ZhichaoGao/edk2/commit/112a41255cb775f5ebede089b > 8b07ba7b836ec44 > I make a minor change of it. But I can't test it because I don't have a = platform > that implement DBG2 table. >=20 > Thanks, > Zhichao >=20 > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Krzysztof Koch > > Sent: Monday, August 5, 2019 4:21 PM > > To: Gao, Zhichao ; devel@edk2.groups.io > > Cc: Carsey, Jaben ; Ni, Ray > > ; Sami Mujawar ; Matteo > > Carlini ; nd > > Subject: Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: DBG2: > > Prevent buffer overruns > > > > Hi Zhichao, > > > > The reason why processing of the Debug Device Information Structure is > > split > > into: > > 1. loading the header > > 2. dumping the entire structure > > > > Is because we want to let the users control how much of the structure > > is dumped. This is important for backward compatibility of the > > acpiview tool with the ACPI specification (and other specs). > > > > New ACPI table fields are appended at the end of structures/tables. > > If, for example, we are asked to parse an old version of Debug Device > > Information Structure, the 'Length' field will tell us to ignore some > > of the newly added fields. These fields do not make sense in the > > context of an old version of the corresponding spec. > > > > The following code in Dbg2Parser.c: > > > > // Make sure the Debug Device Information structure lies inside th= e table. > > if ((Offset + *DbgDevInfoLen) > AcpiTableLength) { > > IncrementErrorCount (); > > Print ( > > L"ERROR: Invalid Debug Device Information structure length. " = \ > > L"DbgDevInfoLen =3D %d. RemainingTableBufferLength =3D %d. "= \ > > L"DBG2 parsing aborted.\n", > > *DbgDevInfoLen, > > AcpiTableLength - Offset > > ); > > return; > > } > > > > Makes sure that the user-provided structure length won't result in a > > buffer overrun with respect to the DBG2 table buffer. This way we > > allow users to specify how much of the structure they want to parse > > while still preventing buffer overruns. > > > > In short, I'm not sure if getting rid of DbgDevInfoHeaderParser would > > work as you assume that the remaining table buffer length should be > > passed to > > ParseAcpi() as an argument, not the length of the Debug Device > > Information Structure. What do you think? > > > > Kind regards, > > > > Krzysztof > > > > -----Original Message----- > > From: Gao, Zhichao > > Sent: Monday, August 5, 2019 7:48 > > 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: DBG2: Prevent buffer > > overruns > > > > About DbgDevInfoHeaderParser and DbgDevInfoParser. > > This patch would parse same DbgDevInfo twice, one for getting length, > > the other for dumping structure info. > > How about the following? > > Add one parameter for DumpDbgDeviceInfo > > > > STATIC > > VOID > > EFIAPI > > DumpDbgDeviceInfo ( > > IN UINT8* Ptr, > > OUT UINT32* Length > > ) > > > > =3D=3D> > > > > STATIC > > VOID > > EFIAPI > > DumpDbgDeviceInfo ( > > IN UINT8* Ptr, > > IN UINT32* Length =09 // remain length of acpi struct to > > parse to make sure all operation is in a valid scope > > OUT UINT16* DbgDevInfoLength // return pointer dbgdevinfo length > > ) > > > > Then we would not need an anditional DbgDevInfoHeaderParser and the > > header would be parsed for only once. > > > > Any better comments, please let me know. > > > > Thanks, > > Zhichao > > > > > -----Original Message----- > > > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com] > > > 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: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer > > > overruns > > > > > > Modify the DBG2 table parsing logic to prevent reading past the ACPI > > > buffer lengths provided. > > > > > > Modify the signature of the DumpDbgDeviceInfo() function to make it > > > consistent with the ACPI structure processing functions in other > > > acpiview parsers. Now, the length of the Debug Device Information > > > Structure is read before the entire structure is dumped. > > > > > > This refactoring change makes it easier to stop reading beyond the > > > DBG2 table buffer if the Debug Device Information Structure Buffer > > > does not fit in the DBG2 buffer. > > > > > > For processing the first two fields of the Debug Device Information > > > Structure (to get the length) a new ACPI_PARSER array is defined. > > > > > > References: > > > - Microsoft Debug Port Table 2 (DBG2), December 10, 2015 > > > > > > Signed-off-by: Krzysztof Koch > > > --- > > > > > > Notes: > > > v1: > > > - Prevent buffer overruns in DBG2 acpiview parser [Krzysztof] > > > > > > > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > > > | 141 +++++++++++++------- > > > 1 file changed, 92 insertions(+), 49 deletions(-) > > > > > > diff --git > > > > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > > r.c > > > > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > > r.c > > > index > > > > > > c6929695a1032c57761ef85002d6c51b7800ce23..869e700b9beda4886bf7bc5ae > > > 4ced3ab9a59efa3 100644 > > > --- > > > > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > > > r.c > > > +++ > > > > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars > > > +++ er.c > > > @@ -64,10 +64,17 @@ STATIC CONST ACPI_PARSER Dbg2Parser[] =3D { > > > (VOID**)&NumberDbgDeviceInfo, NULL, NULL} }; > > > > > > +/// An ACPI_PARSER array describing the debug device information > > > +structure /// header. > > > +STATIC CONST ACPI_PARSER DbgDevInfoHeaderParser[] =3D { > > > + {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL}, > > > + {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, > > > +NULL} }; > > > + > > > /// An ACPI_PARSER array describing the debug device information. > > > STATIC CONST ACPI_PARSER DbgDevInfoParser[] =3D { > > > {L"Revision", 1, 0, L"0x%x", NULL, NULL, NULL, NULL}, > > > - {L"Length", 2, 1, L"%d", NULL, (VOID**)&DbgDevInfoLen, NULL, > > > NULL}, > > > + {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL}, > > > > > > {L"Generic Address Registers Count", 1, 3, L"0x%x", NULL, > > > (VOID**)&GasCount, NULL, NULL}, > > > @@ -93,76 +100,91 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] > =3D > > { > > > /** > > > This function parses the debug device information structure. > > > > > > - @param [in] Ptr Pointer to the start of the buffer. > > > - @param [out] Length Pointer in which the length of the debug > > > - device information is returned. > > > + @param [in] Ptr Pointer to the start of the buffer. > > > + @param [in] Length Length of the debug device information struct= ure. > > > **/ > > > STATIC > > > VOID > > > EFIAPI > > > DumpDbgDeviceInfo ( > > > - IN UINT8* Ptr, > > > - OUT UINT32* Length > > > + IN UINT8* Ptr, > > > + IN UINT16 Length > > > ) > > > { > > > UINT16 Index; > > > - UINT8* DataPtr; > > > - UINT32* AddrSize; > > > - > > > - // Parse the debug device info to get the Length > > > - ParseAcpi ( > > > - FALSE, > > > - 0, > > > - "Debug Device Info", > > > - Ptr, > > > - 3, // Length is 2 bytes starting at offset 1 > > > - PARSER_PARAMS (DbgDevInfoParser) > > > - ); > > > + UINT16 Offset; > > > > > > ParseAcpi ( > > > TRUE, > > > 2, > > > "Debug Device Info", > > > Ptr, > > > - *DbgDevInfoLen, > > > + Length, > > > PARSER_PARAMS (DbgDevInfoParser) > > > ); > > > > > > - // GAS and Address Size > > > + // GAS > > > Index =3D 0; > > > - DataPtr =3D Ptr + (*BaseAddrRegOffset); > > > - AddrSize =3D (UINT32*)(Ptr + (*AddrSizeOffset)); > > > - while (Index < (*GasCount)) { > > > + Offset =3D *BaseAddrRegOffset; > > > + while ((Index++ < *GasCount) && > > > + (Offset < Length)) { > > > PrintFieldName (4, L"BaseAddressRegister"); > > > - DumpGasStruct (DataPtr, 4, GAS_LENGTH); > > > + Offset +=3D (UINT16)DumpGasStruct ( > > > + Ptr + Offset, > > > + 4, > > > + Length - Offset > > > + ); > > > + } > > > + > > > + // Make sure the array of address sizes corresponding to each GAS > > > + fit in the // Debug Device Information structure if > > > + ((*AddrSizeOffset + (*GasCount * sizeof (UINT32))) > Length) { > > > + IncrementErrorCount (); > > > + Print ( > > > + L"ERROR: Invalid GAS count. GasCount =3D %d. > > > + RemainingBufferLength > > > =3D %d. " \ > > > + L"Parsing of the Debug Device Information structure aborted= .\n", > > > + *GasCount, > > > + Length - *AddrSizeOffset > > > + ); > > > + return; > > > + } > > > + > > > + // Address Size > > > + Index =3D 0; > > > + Offset =3D *AddrSizeOffset; > > > + while ((Index++ < *GasCount) && > > > + (Offset < Length)) { > > > PrintFieldName (4, L"Address Size"); > > > - Print (L"0x%x\n", AddrSize[Index]); > > > - DataPtr +=3D GAS_LENGTH; > > > - Index++; > > > + Print (L"0x%x\n", *((UINT32*)(Ptr + Offset))); > > > + Offset +=3D sizeof (UINT32); > > > } > > > > > > // NameSpace String > > > Index =3D 0; > > > - DataPtr =3D Ptr + (*NameSpaceStringOffset); > > > + Offset =3D *NameSpaceStringOffset; > > > PrintFieldName (4, L"NameSpace String"); > > > - while (Index < (*NameSpaceStringLength)) { > > > - Print (L"%c", DataPtr[Index++]); > > > + while ((Index++ < *NameSpaceStringLength) && > > > + (Offset < Length)) { > > > + Print (L"%c", *(Ptr + Offset)); > > > + Offset++; > > > } > > > Print (L"\n"); > > > > > > // OEM Data > > > - Index =3D 0; > > > - DataPtr =3D Ptr + (*OEMDataOffset); > > > - PrintFieldName (4, L"OEM Data"); > > > - while (Index < (*OEMDataLength)) { > > > - Print (L"%x ", DataPtr[Index++]); > > > - if ((Index & 7) =3D=3D 0) { > > > - Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L""); > > > + if (*OEMDataOffset !=3D 0) { > > > + Index =3D 0; > > > + Offset =3D *OEMDataOffset; > > > + PrintFieldName (4, L"OEM Data"); > > > + while ((Index++ < *OEMDataLength) && > > > + (Offset < Length)) { > > > + Print (L"%x ", *(Ptr + Offset)); > > > + if ((Index & 7) =3D=3D 0) { > > > + Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L""); > > > + } > > > + Offset++; > > > } > > > + Print (L"\n"); > > > } > > > - Print (L"\n"); > > > - > > > - *Length =3D *DbgDevInfoLen; > > > } > > > > > > /** > > > @@ -187,8 +209,7 @@ ParseAcpiDbg2 ( > > > ) > > > { > > > UINT32 Offset; > > > - UINT32 DbgDeviceInfoLength; > > > - UINT8* DevInfoPtr; > > > + UINT32 Index; > > > > > > if (!Trace) { > > > return; > > > @@ -202,14 +223,36 @@ ParseAcpiDbg2 ( > > > AcpiTableLength, > > > PARSER_PARAMS (Dbg2Parser) > > > ); > > > - DevInfoPtr =3D Ptr + Offset; > > > > > > - while (Offset < AcpiTableLength) { > > > - DumpDbgDeviceInfo ( > > > - DevInfoPtr, > > > - &DbgDeviceInfoLength > > > + Offset =3D *OffsetDbgDeviceInfo; > > > + Index =3D 0; > > > + > > > + while (Index++ < *NumberDbgDeviceInfo) { > > > + > > > + // Parse the Debug Device Information Structure header to > > > + obtain > > Length > > > + ParseAcpi ( > > > + FALSE, > > > + 0, > > > + NULL, > > > + Ptr + Offset, > > > + AcpiTableLength - Offset, > > > + PARSER_PARAMS (DbgDevInfoHeaderParser) > > > ); > > > - Offset +=3D DbgDeviceInfoLength; > > > - DevInfoPtr +=3D DbgDeviceInfoLength; > > > + > > > + // Make sure the Debug Device Information structure lies inside > > > + the > > table. > > > + if ((Offset + *DbgDevInfoLen) > AcpiTableLength) { > > > + IncrementErrorCount (); > > > + Print ( > > > + L"ERROR: Invalid Debug Device Information structure length.= " \ > > > + L"DbgDevInfoLen =3D %d. RemainingTableBufferLength =3D %d= . " \ > > > + L"DBG2 parsing aborted.\n", > > > + *DbgDevInfoLen, > > > + AcpiTableLength - Offset > > > + ); > > > + return; > > > + } > > > + > > > + DumpDbgDeviceInfo (Ptr + Offset, (*DbgDevInfoLen)); > > > + Offset +=3D (*DbgDevInfoLen); > > > } > > > } > > > -- > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > > > > >=20 >=20 >=20 >=20 >=20 >=20