I got your point. How about this: https://github.com/ZhichaoGao/edk2/commit/112a41255cb775f5ebede089b8b07ba7b836ec44 I make a minor change of it. But I can't test it because I don't have a platform that implement DBG2 table.
Thanks, Zhichao > -----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 <zhichao....@intel.com>; devel@edk2.groups.io > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; > Sami Mujawar <sami.muja...@arm.com>; Matteo Carlini > <matteo.carl...@arm.com>; nd <n...@arm.com> > 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 the table. > if ((Offset + *DbgDevInfoLen) > AcpiTableLength) { > IncrementErrorCount (); > Print ( > L"ERROR: Invalid Debug Device Information structure length. " \ > L"DbgDevInfoLen = %d. RemainingTableBufferLength = %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 <zhichao....@intel.com> > Sent: Monday, August 5, 2019 7:48 > To: Krzysztof Koch <krzysztof.k...@arm.com>; devel@edk2.groups.io > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; > Sami Mujawar <sami.muja...@arm.com>; Matteo Carlini > <matteo.carl...@arm.com>; nd <n...@arm.com> > 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 > ) > > ==> > > STATIC > VOID > EFIAPI > DumpDbgDeviceInfo ( > IN UINT8* Ptr, > IN UINT32* Length // 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.k...@arm.com] > > Sent: Thursday, August 1, 2019 4:44 PM > > To: devel@edk2.groups.io > > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray > > <ray...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; > > sami.muja...@arm.com; matteo.carl...@arm.com; n...@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 <krzysztof.k...@arm.com> > > --- > > > > 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[] = { > > (VOID**)&NumberDbgDeviceInfo, NULL, NULL} }; > > > > +/// An ACPI_PARSER array describing the debug device information > > +structure /// header. > > +STATIC CONST ACPI_PARSER DbgDevInfoHeaderParser[] = { > > + {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[] = { > > {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[] = > { > > /** > > 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 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; > > > > ParseAcpi ( > > TRUE, > > 2, > > "Debug Device Info", > > Ptr, > > - *DbgDevInfoLen, > > + Length, > > PARSER_PARAMS (DbgDevInfoParser) > > ); > > > > - // GAS and Address Size > > + // GAS > > Index = 0; > > - DataPtr = Ptr + (*BaseAddrRegOffset); > > - AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset)); > > - while (Index < (*GasCount)) { > > + Offset = *BaseAddrRegOffset; > > + while ((Index++ < *GasCount) && > > + (Offset < Length)) { > > PrintFieldName (4, L"BaseAddressRegister"); > > - DumpGasStruct (DataPtr, 4, GAS_LENGTH); > > + Offset += (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 = %d. > > + RemainingBufferLength > > = %d. " \ > > + L"Parsing of the Debug Device Information structure aborted.\n", > > + *GasCount, > > + Length - *AddrSizeOffset > > + ); > > + return; > > + } > > + > > + // Address Size > > + Index = 0; > > + Offset = *AddrSizeOffset; > > + while ((Index++ < *GasCount) && > > + (Offset < Length)) { > > PrintFieldName (4, L"Address Size"); > > - Print (L"0x%x\n", AddrSize[Index]); > > - DataPtr += GAS_LENGTH; > > - Index++; > > + Print (L"0x%x\n", *((UINT32*)(Ptr + Offset))); > > + Offset += sizeof (UINT32); > > } > > > > // NameSpace String > > Index = 0; > > - DataPtr = Ptr + (*NameSpaceStringOffset); > > + Offset = *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 = 0; > > - DataPtr = Ptr + (*OEMDataOffset); > > - PrintFieldName (4, L"OEM Data"); > > - while (Index < (*OEMDataLength)) { > > - Print (L"%x ", DataPtr[Index++]); > > - if ((Index & 7) == 0) { > > - Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L""); > > + if (*OEMDataOffset != 0) { > > + Index = 0; > > + Offset = *OEMDataOffset; > > + PrintFieldName (4, L"OEM Data"); > > + while ((Index++ < *OEMDataLength) && > > + (Offset < Length)) { > > + Print (L"%x ", *(Ptr + Offset)); > > + if ((Index & 7) == 0) { > > + Print (L"\n%-*s ", OUTPUT_FIELD_COLUMN_WIDTH, L""); > > + } > > + Offset++; > > } > > + Print (L"\n"); > > } > > - Print (L"\n"); > > - > > - *Length = *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 = Ptr + Offset; > > > > - while (Offset < AcpiTableLength) { > > - DumpDbgDeviceInfo ( > > - DevInfoPtr, > > - &DbgDeviceInfoLength > > + Offset = *OffsetDbgDeviceInfo; > > + Index = 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 += DbgDeviceInfoLength; > > - DevInfoPtr += 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 = %d. RemainingTableBufferLength = %d. " \ > > + L"DBG2 parsing aborted.\n", > > + *DbgDevInfoLen, > > + AcpiTableLength - Offset > > + ); > > + return; > > + } > > + > > + DumpDbgDeviceInfo (Ptr + Offset, (*DbgDevInfoLen)); > > + Offset += (*DbgDevInfoLen); > > } > > } > > -- > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44937): https://edk2.groups.io/g/devel/message/44937 Mute This Topic: https://groups.io/mt/32676831/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
