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 00:00:16 -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; 04 Aug 2019 23:48:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,348,1559545200"; d="scan'208";a="181595810" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 04 Aug 2019 23:48:32 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 4 Aug 2019 23:48:31 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.80]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.65]) with mapi id 14.03.0439.000; Mon, 5 Aug 2019 14:48:28 +0800 From: "Gao, Zhichao" To: Krzysztof Koch , "devel@edk2.groups.io" CC: "Carsey, Jaben" , "Ni, Ray" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Thread-Topic: [PATCH v1 1/6] ShellPkg: acpiview: DBG2: Prevent buffer overruns Thread-Index: AQHVSEVpsSE81gr5hk6xm4Bgvoopw6bsHvjg Date: Mon, 5 Aug 2019 06:48:27 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B81EA99@SHSMSX101.ccr.corp.intel.com> References: <20190801084407.48712-1-krzysztof.koch@arm.com> <20190801084407.48712-2-krzysztof.koch@arm.com> In-Reply-To: <20190801084407.48712-2-krzysztof.koch@arm.com> 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 About DbgDevInfoHeaderParser and DbgDevInfoParser. This patch would parse same DbgDevInfo twice, one for getting length, the o= ther 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 // remain length of acpi struct to parse to make sur= e 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 >=20 > Modify the DBG2 table parsing logic to prevent reading past the ACPI buff= er > lengths provided. >=20 > 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 rea= d > before the entire structure is dumped. >=20 > This refactoring change makes it easier to stop reading beyond the > DBG2 table buffer if the Debug Device Information Structure Buffer does n= ot > fit in the DBG2 buffer. >=20 > For processing the first two fields of the Debug Device Information Struc= ture > (to get the length) a new ACPI_PARSER array is defined. >=20 > References: > - Microsoft Debug Port Table 2 (DBG2), December 10, 2015 >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Notes: > v1: > - Prevent buffer overruns in DBG2 acpiview parser [Krzysztof] >=20 >=20 > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > | 141 +++++++++++++------- > 1 file changed, 92 insertions(+), 49 deletions(-) >=20 > 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} }; >=20 > +/// 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}, >=20 > {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. >=20 > - @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 structure. > **/ > 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; >=20 > ParseAcpi ( > TRUE, > 2, > "Debug Device Info", > Ptr, > - *DbgDevInfoLen, > + Length, > PARSER_PARAMS (DbgDevInfoParser) > ); >=20 > - // 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); > } >=20 > // 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"); >=20 > // 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; > } >=20 > /** > @@ -187,8 +209,7 @@ ParseAcpiDbg2 ( > ) > { > UINT32 Offset; > - UINT32 DbgDeviceInfoLength; > - UINT8* DevInfoPtr; > + UINT32 Index; >=20 > if (!Trace) { > return; > @@ -202,14 +223,36 @@ ParseAcpiDbg2 ( > AcpiTableLength, > PARSER_PARAMS (Dbg2Parser) > ); > - DevInfoPtr =3D Ptr + Offset; >=20 > - while (Offset < AcpiTableLength) { > - DumpDbgDeviceInfo ( > - DevInfoPtr, > - &DbgDeviceInfoLength > + Offset =3D *OffsetDbgDeviceInfo; > + Index =3D 0; > + > + while (Index++ < *NumberDbgDeviceInfo) { > + > + // Parse the Debug Device Information Structure header to obtain Len= gth > + 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