[PATCH v1 11/11] ShellPkg: acpiview: DBG2: Add error-checking in the parsing logic

["Krzysztof Koch" <krzysztof.koch@arm.com>]

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.koch@arm.com>
---

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/530b059a9fe4aa9f1df36b407f97d76acaab8b74

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..2d6ff80e299eebe7853061d3db89332197c0dc0e 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..20ca358bddfa5953bfb1d1bebaebbf3079eaba01 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/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2bbd622ffb7cec0a340de3e10bdcd01ba4d330df 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.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);
   }
 
   // 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)
+      );
+
+    // 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)'