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

Krzysztof Koch posted 11 patches 32 weeks ago

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

Posted by Krzysztof Koch 32 weeks ago
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)'



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43651): https://edk2.groups.io/g/devel/message/43651
Mute This Topic: https://groups.io/mt/32439516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Gao, Zhichao 31 weeks ago
Two comments with this patch.

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Friday, July 12, 2019 2:53 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Sami.Mujawar@arm.com;
> Matteo.Carlini@arm.com; nd@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.koch@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Alexei Fedorov 31 weeks ago
Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#43863): https://edk2.groups.io/g/devel/message/43863
Mute This Topic: https://groups.io/mt/32439516/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-