Two comments with this patch. > -----Original Message----- > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > Sent: Friday, July 12, 2019 2:53 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 11/11] ShellPkg: acpiview: DBG2: Add error-checking in > the parsing logic > > 1. Check if the global pointers (in the scope of this ACPI table parser) have > been successfully updated before they are later used to control the parsing > logic. > > 2. Remove redundant forward function declarations by repositioning blocks > of code. > > 3. Test against buffer overruns. > > 4. Introduce a ACPI_PARSER array for parsing the header of the debug device > information structure. This way, the length of the buffer storing a debug > device information structure instance can be passed to > DumpDbgDeviceInfo(). Consequently, the parsing logic becomes consistent > with other ACPI table parsers and tests against buffer overrruns are simpler > to implement. > > 5. Modify the signature of DumpGasStruct() function inside AcpiParser.c to > facilitate protection against buffer overruns in the DBG2 parser. > > Signed-off-by: Krzysztof Koch <krzysztof.k...@arm.com> > --- > > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/commit/530b059a9fe4aa9f1df36b4 > 07f97d76acaab8b74 > > Notes: > v1: > - improve the logic in the DBG2 parser [Krzysztof] > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 26 > +- > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 8 > +- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > | 298 ++++++++++++++------ > 3 files changed, 225 insertions(+), 107 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[] = { > > @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..2bbd622ffb7cec0a340de3e10 > bdcd01ba4d330df 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > r.c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars > +++ er.c > @@ -12,6 +12,7 @@ > #include <Library/UefiLib.h> > #include "AcpiParser.h" > #include "AcpiTableParser.h" > +#include "AcpiView.h" > > // Local variables pointing to the table fields STATIC CONST UINT32* > OffsetDbgDeviceInfo; @@ -27,7 +28,7 @@ STATIC CONST UINT16* > AddrSizeOffset; STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo; > > /** > - This function Validates the NameSpace string length. > + This function validates the NameSpace string length. > > @param [in] Ptr Pointer to the start of the buffer. > @param [in] Context Pointer to context specific information e.g. this @@ - > 37,24 +38,23 @@ STATIC VOID EFIAPI ValidateNameSpaceStrLen ( > - IN UINT8* Ptr, > - IN VOID* Context > - ); > + IN UINT8* Ptr, > + IN VOID* Context > + ) > +{ > + UINT16 NameSpaceStrLen; > > -/** > - This function parses the debug device information structure. > + NameSpaceStrLen = *(UINT16*)Ptr; > > - @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. > -**/ > -STATIC > -VOID > -EFIAPI > -DumpDbgDeviceInfo ( > - IN UINT8* Ptr, > - OUT UINT32* Length > - ); > + if (NameSpaceStrLen < 2) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: NamespaceString Length = %d. If no Namespace device > exists, " \ > + L"NamespaceString[] must contain a period '.'", > + NameSpaceStrLen > + ); > + } > +} > > /// An ACPI_PARSER array describing the ACPI DBG2 table. > STATIC CONST ACPI_PARSER Dbg2Parser[] = { @@ -65,10 +65,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}, > @@ -91,108 +98,152 @@ STATIC CONST ACPI_PARSER DbgDevInfoParser[] = > { > (VOID**)&AddrSizeOffset, NULL, NULL} }; > > -/** > - This function validates the NameSpace string length. > - > - @param [in] Ptr Pointer to the start of the buffer. > - @param [in] Context Pointer to context specific information e.g. this > - could be a pointer to the ACPI table header. > -**/ > -STATIC > -VOID > -EFIAPI > -ValidateNameSpaceStrLen ( > - IN UINT8* Ptr, > - IN VOID* Context > - ) > -{ > - UINT16 NameSpaceStrLen; > - > - NameSpaceStrLen = *(UINT16*)Ptr; > - > - if (NameSpaceStrLen < 2) { > - IncrementErrorCount (); > - Print ( > - L"\nERROR: NamespaceString Length = %d. If no Namespace device > exists,\n" > - L" then NamespaceString[] must contain a period '.'", > - NameSpaceStrLen > - ); > - } > -} > - > /** > 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 > + // Check if the values used to control the parsing logic have been > + // successfully read. > + if ((GasCount == NULL) || > + (NameSpaceStringLength == NULL) || > + (NameSpaceStringOffset == NULL) || > + (OEMDataLength == NULL) || > + (OEMDataOffset == NULL) || > + (BaseAddrRegOffset == NULL) || > + (AddrSizeOffset == NULL)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Insufficient Debug Device Information Structure length. " \ > + L"Length = %d.\n", > + Length > + ); > + return; > + } > + > + // 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); > + Offset += (UINT16)DumpGasStruct ( > + Ptr + Offset, > + 4, > + Length - Offset > + ); > + } > + > + // Cross-check the substructure count with the length of the > + encapsulating // buffer if (GetConsistencyChecking () && > + (Index < *GasCount)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid GAS count. GasCount = %d. DbgDevInfoLen = %d.\n", > + *GasCount, > + Length > + ); > + return; > + } > + > + // 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);
1. Why use sizof (UINT32) to replace GAS_LENGTH? The MACRO make a good view and meaningful. > } > > // 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"); > > + // Cross-check the string length with the size of the encapsulating > + // buffer if (GetConsistencyChecking () && > + (Index < *NameSpaceStringLength)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid NameSpaceString length. NameSpaceStringLength = %d. > " \ > + L"DbgDevInfoLen = %d.\n", > + *NameSpaceStringLength, > + Length > + ); > + return; > + } > + > // 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; > + // Cross-check the OEM data length with the size of the encapsulating > + // buffer if (GetConsistencyChecking () && > + (Index < *OEMDataLength)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid OEM Data size. OEMDataLength = %d. " \ > + L"DbgDevInfoLen = %d.\n", > + *OEMDataLength, > + Length > + ); > + } > } > > /** > @@ -217,8 +268,7 @@ ParseAcpiDbg2 ( > ) > { > UINT32 Offset; > - UINT32 DbgDeviceInfoLength; > - UINT8* DevInfoPtr; > + UINT32 Index; > > if (!Trace) { > return; > @@ -232,14 +282,74 @@ ParseAcpiDbg2 ( > AcpiTableLength, > PARSER_PARAMS (Dbg2Parser) > ); > - DevInfoPtr = Ptr + Offset; > > - while (Offset < AcpiTableLength) { > - DumpDbgDeviceInfo ( > - DevInfoPtr, > - &DbgDeviceInfoLength > + // Check if the values used to control the parsing logic have been > + // successfully read. > + if ((OffsetDbgDeviceInfo == NULL) || > + (NumberDbgDeviceInfo == NULL)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Insufficient table length. AcpiTableLength = %d\n", > + AcpiTableLength > + ); > + return; > + } > + > + 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) > + ); 2. Here adding the DbgDevInfoHeaderParser to get the DbgDevInfoLen. This may be conflict with 4/10. 4/10 removes the get header section. I think it is fine to get the whole devinfo if the Length is valid. And the DbgDevInfoHeaderParser isn't required to add. Thanks, Zhichao > + > + // Check if the values used to control the parsing logic have been > + // successfully read. > + if (DbgDevInfoLen == NULL) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Insufficient remaining table buffer length to read the " \ > + L"Debug Device Information structure's 'Length' field. " \ > + L"RemainingTableBufferLength = %d.\n", > + AcpiTableLength - Offset > + ); > + return; > + } > + > + // 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); > + } > + > + // Cross-check the substructure count with the length of the > + encapsulating // buffer if (GetConsistencyChecking () && > + (Index < *NumberDbgDeviceInfo)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Invalid Debug Device Information structure count. " \ > + L"NumberDbgDeviceInfo = %d. AcpiTableLength = %d.\n", > + *NumberDbgDeviceInfo, > + AcpiTableLength > ); > - Offset += DbgDeviceInfoLength; > - DevInfoPtr += DbgDeviceInfoLength; > } > } > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44004): https://edk2.groups.io/g/devel/message/44004 Mute This Topic: https://groups.io/mt/32439516/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-