[PATCH v3 00/11] Test against invalid pointers in acpiview

[PATCH v3 00/11] Test against invalid pointers in acpiview

[Krzysztof Koch]

Prevent the use of invalid pointers when parsing ACPI tables in the UEFI
shell acpiview tool.

The parsing of ACPI tables is often controlled with the values read
earlier from the same table. For example, the 'Offset' or 'Count' fields
found in a structure are later used to parse the substructures. If such
fields lie outside the structure's buffer length provided, then there
is a possibility for a wild or dangling pointer.

Currently, if the ParseAcpi() function terminates early because the end
of the input table data buffer has been reached, then the pointers
which were supposed to be updated by this function are left untouched.
This is a security issue as the values pointed to by these pointers are
later used for flow control.

This patch series aims to solve this security issue by explicitly
initializing any pointers lying outside the input ACPI data buffer to
NULL and testing for NULL whenever these pointers are dereferenced.

Changes can be seet at: https://github.com/KrzysztofKoch1/edk2/tree/612_add_pointer_validation_v3

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v2:
    - Do not require FadtMinorRevision and X_DsdtAddress pointers to be
      valid in FADT table parser [Zhichao]

    v1:
    - Validate static pointers in acpiview parsers before use [Krzysztof]

Krzysztof Koch (11):
  ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields
  ShellPkg: acpiview: RSDP: Validate global pointer before use
  ShellPkg: acpiview: FADT: Validate global pointer before use
  ShellPkg: acpiview: SLIT: Validate global pointer before use
  ShellPkg: acpiview: SLIT: Validate System Locality count
  ShellPkg: acpiview: SRAT: Validate global pointers before use
  ShellPkg: acpiview: MADT: Validate global pointers before use
  ShellPkg: acpiview: PPTT: Validate global pointers before use
  ShellPkg: acpiview: IORT: Validate global pointers before use
  ShellPkg: acpiview: GTDT: Validate global pointers before use
  ShellPkg: acpiview: DBG2: Validate global pointers before use

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              |  9 ++-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 43 ++++++++++++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 21 +++----
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37 ++++++++++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52 +++++++++++++++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 13 +++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25 ++++++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 12 ++++
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 61 ++++++++++++++++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13 +++++
 10 files changed, 269 insertions(+), 17 deletions(-)

--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 01/11] ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields

[Krzysztof Koch]

For fields outside the buffer length provided, reset any pointers,
which were supposed to be updated by a ParseAcpi() function call to
NULL. This way one can easily validate if a pointer was successfully
updated.

The ParseAcpi() function parses the given ACPI table buffer by a
number of bytes which is a minimum of the buffer length and the length
described by ACPI_PARSER array. If the buffer length is shorter than
the array describing how to process the ACPI structure, then it is
possible that the ItemPtr inside ACPI_PARSER may not get updated or
initialized. This can lead to an error if the value pointed to by
ItemPtr is later used to control the parsing logic.

A typical example would be a 'number of elements' field in an ACPI
structure header which defines how many substructures of a given type
are present in the structure body. If the 'number of elements' field
is not parsed, we will have a dangling pointer which could cause a
problem later.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Set ItemPtr to NULL for unprocessed table fields [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
index 2b2ecb93cef9ee28b752e7bf2d920b059dbf7d6b..84c5f0468da55477acc96dfd0f949a5908d0f7a5 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
@@ -543,8 +543,15 @@ ParseAcpi (
 
   for (Index = 0; Index < ParserItems; Index++) {
     if ((Offset + Parser[Index].Length) > Length) {
+
+      // For fields outside the buffer length provided, reset any pointers
+      // which were supposed to be updated by this function call
+      if (Parser[Index].ItemPtr != NULL) {
+        *Parser[Index].ItemPtr = NULL;
+      }
+
       // We don't parse past the end of the max length specified
-      break;
+      continue;
     }
 
     if (GetConsistencyChecking () &&
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 02/11] ShellPkg: acpiview: RSDP: Validate global pointer before use

[Krzysztof Koch]

Check if XsdtAddress pointer has been successfully updated before it
is used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index 5a5c4b50c12e6eb0aa0efb1765df7e123f614da3..f4a8732a7db7c437031f2a3d2f266b80eff17b4b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -138,6 +138,18 @@ ParseAcpiRsdp (
     PARSER_PARAMS (RsdpParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if (XsdtAddress == NULL) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d." \
+        L"RSDP parsing aborted.\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   // This code currently supports parsing of XSDT table only
   // and does not parse the RSDT table. Platforms provide the
   // RSDT to enable compatibility with ACPI 1.0 operating systems.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 03/11] ShellPkg: acpiview: FADT: Validate global pointer before use

[Krzysztof Koch]

Check if global pointers have been successfully updated before they
are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v2:
    - Do not require FadtMinorRevision and X_DsdtAddress pointers to be
      valid in order to process the remaining ACPI tables [Zhichao]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 21 ++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index 5b8cc174f16afb3d4feb6a518952e60c6564ee34..37cbd8be287944656afcd609a3dd080440d5cfef 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -1,7 +1,7 @@
 /** @file
   FADT table parser
 
-  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
+  Copyright (c) 2016 - 2020, ARM Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -230,9 +230,11 @@ ParseAcpiFadt (
     );
 
   if (Trace) {
-    Print (L"\nSummary:\n");
-    PrintFieldName (2, L"FADT Version");
-    Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
+    if (FadtMinorRevision != NULL) {
+      Print (L"\nSummary:\n");
+      PrintFieldName (2, L"FADT Version");
+      Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
+    }
 
     if (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId) {
       IncrementErrorCount ();
@@ -294,21 +296,20 @@ ParseAcpiFadt (
       );
   }
 
-  // If X_DSDT is not zero then use X_DSDT and ignore DSDT,
-  // else use DSDT.
-  if (*X_DsdtAddress != 0) {
+  // If X_DSDT is valid then use X_DSDT and ignore DSDT, else use DSDT.
+  if ((X_DsdtAddress != NULL) && (*X_DsdtAddress != 0)) {
     DsdtPtr = (UINT8*)(UINTN)(*X_DsdtAddress);
-  } else if (*DsdtAddress != 0) {
+  } else if ((DsdtAddress != NULL) && (*DsdtAddress != 0)) {
     DsdtPtr = (UINT8*)(UINTN)(*DsdtAddress);
   } else {
-    // Both DSDT and X_DSDT cannot be zero.
+    // Both DSDT and X_DSDT cannot be invalid.
 #if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
     if (Trace) {
       // The DSDT Table is mandatory for ARM systems
       // as the CPU information MUST be presented in
       // the DSDT.
       IncrementErrorCount ();
-      Print (L"ERROR: Both X_DSDT and DSDT are NULL.\n");
+      Print (L"ERROR: Both X_DSDT and DSDT are invalid.\n");
     }
 #endif
     return;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 04/11] ShellPkg: acpiview: SLIT: Validate global pointer before use

[Krzysztof Koch]

Check if SlitSystemLocalityCount pointer has been successfully updated
before it is used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index ca2808db526b1bbb79aeb21ccfc0ae6c79b2dfd8..17e2166a09d8615b714e0c51d4d93d293fcdf601 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -1,7 +1,7 @@
 /** @file
   SLIT table parser
 
-  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Reference(s):
@@ -75,9 +75,21 @@ ParseAcpiSlit (
              AcpiTableLength,
              PARSER_PARAMS (SlitParser)
              );
+
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if (SlitSystemLocalityCount == NULL) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   LocalityPtr = Ptr + Offset;
-
   LocalityCount = *SlitSystemLocalityCount;
+
   // We only print the Localities if the count is less than 16
   // If the locality count is more than 16 then refer to the
   // raw data dump.
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

[Krzysztof Koch]

1. Check if the 'Number of System Localities' provided can be
represented in the SLIT table. The table 'Length' field is a 32-bit
value while the 'Number of System Localities' field is 64-bit long.

2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT
locality count, then the matrix used to represent the localities is
N*N bytes long. The ACPI table length must be big enough to fit the
matrix.

3. Remove (now) redundant 64x64 bit multiplication.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Validate the 'Number of System Localities' Field [Krzysztof]
    - Remove redundant 64x64 bit multiplication [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 47 +++++++++++++++++---
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990ecb956baf91cc3b9 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
@@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
 /**
   Macro to get the value of a System Locality
 **/
-#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j)
+#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j)
 
 /**
   This function parses the ACPI SLIT table.
@@ -57,9 +57,9 @@ ParseAcpiSlit (
   )
 {
   UINT32 Offset;
-  UINT64 Count;
-  UINT64 Index;
-  UINT64 LocalityCount;
+  UINT32 Count;
+  UINT32 Index;
+  UINT32 LocalityCount;
   UINT8* LocalityPtr;
   CHAR16 Buffer[80];  // Used for AsciiName param of ParseAcpi
 
@@ -87,8 +87,45 @@ ParseAcpiSlit (
     return;
   }
 
+  /*
+    Despite the 'Number of System Localities' being a 64-bit field in SLIT,
+    the maximum number of localities that can be represented in SLIT is limited
+    by the 'Length' field of the ACPI table.
+
+    Since the ACPI table length field is 32-bit wide. The maximum number of
+    localities that can be represented in SLIT can be calculated as:
+
+    MaxLocality = sqrt (MAX_UINT32 - sizeof (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEADER))
+                = 65535
+                = MAX_UINT16
+  */
+  if (*SlitSystemLocalityCount > MAX_UINT16) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: The Number of System Localities provided can't be represented " \
+        L"in the SLIT table. SlitSystemLocalityCount = %ld. " \
+        L"MaxLocalityCountAllowed = %d.\n",
+      *SlitSystemLocalityCount,
+      MAX_UINT16
+      );
+    return;
+  }
+
+  LocalityCount = (UINT32)*SlitSystemLocalityCount;
+
+  // Make sure system localities fit in the table buffer provided
+  if (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid Number of System Localities. " \
+        L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n",
+      *SlitSystemLocalityCount,
+      AcpiTableLength
+      );
+    return;
+  }
+
   LocalityPtr = Ptr + Offset;
-  LocalityCount = *SlitSystemLocalityCount;
 
   // We only print the Localities if the count is less than 16
   // If the locality count is more than 16 then refer to the
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 06/11] ShellPkg: acpiview: SRAT: Validate global pointers before use

[Krzysztof Koch]

Check if SratRAType and SratRALength pointers have been successfully
updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 6fe7bf681132df08133e3e03e3ee3f020d905dd2..3613900ae322483fdd3d3383de4e22ba75b2128b 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -399,6 +399,19 @@ ParseAcpiSrat (
       PARSER_PARAMS (SratResourceAllocationParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((SratRAType == NULL) ||
+        (SratRALength == NULL)) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"Static Resource Allocation structure header. Length = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure the SRAT structure lies inside the table
     if ((Offset + *SratRALength) > AcpiTableLength) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 07/11] ShellPkg: acpiview: MADT: Validate global pointers before use

[Krzysztof Koch]

Check if the MadtInterruptControllerType and
MadtInterruptControllerLength pointers have been successfully updated
before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 90bdafea1970db522e8ed96de7c6e986cdaca5ba..438905cb24f58b8b82e8fe61280e72f765d578d8 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -260,6 +260,19 @@ ParseAcpiMadt (
       PARSER_PARAMS (MadtInterruptControllerHeaderParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((MadtInterruptControllerType == NULL) ||
+        (MadtInterruptControllerLength == NULL)) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"Interrupt Controller Structure header. Length = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure forward progress is made.
     if (*MadtInterruptControllerLength < 2) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 08/11] ShellPkg: acpiview: PPTT: Validate global pointers before use

[Krzysztof Koch]

Check if the NumberOfPrivateResources, ProcessorTopologyStructureType
and ProcessorTopologyStructureLength pointers have been successfully
updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25 ++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index 6254b9913fffb429fc54bb1301bf3e4b2e5bf161..675ba75f02b367cd5ad9f2ac23c30ed0ab58f286 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -264,6 +264,17 @@ DumpProcessorHierarchyNodeStructure (
              PARSER_PARAMS (ProcessorHierarchyNodeStructureParser)
              );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if (NumberOfPrivateResources == NULL) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient Processor Hierarchy Node length. Length = %d.\n",
+      Length
+      );
+    return;
+  }
+
   // Make sure the Private Resource array lies inside this structure
   if (Offset + (*NumberOfPrivateResources * sizeof (UINT32)) > Length) {
     IncrementErrorCount ();
@@ -387,6 +398,7 @@ ParseAcpiPptt (
              AcpiTableLength,
              PARSER_PARAMS (PpttParser)
              );
+
   ProcessorTopologyStructurePtr = Ptr + Offset;
 
   while (Offset < AcpiTableLength) {
@@ -400,6 +412,19 @@ ParseAcpiPptt (
       PARSER_PARAMS (ProcessorTopologyStructureHeaderParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((ProcessorTopologyStructureType == NULL) ||
+        (ProcessorTopologyStructureLength == NULL)) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"processor topology structure header. Length = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure the PPTT structure lies inside the table
     if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 09/11] ShellPkg: acpiview: IORT: Validate global pointers before use

[Krzysztof Koch]

Check if global (in the scope of the IORT parser) pointers have been
successfully updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52 ++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 72289c7680bc3cd5c444481e8d6a719803202a9b..9d5d937c7b2c19945ca2ad3eba644bdfc09cc3f6 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -322,6 +322,20 @@ DumpIortNodeSmmuV1V2 (
     PARSER_PARAMS (IortNodeSmmuV1V2Parser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((InterruptContextCount == NULL)   ||
+      (InterruptContextOffset == NULL)  ||
+      (PmuInterruptCount == NULL)       ||
+      (PmuInterruptOffset == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient SMMUv1/2 node length. Length = %d\n",
+      Length
+      );
+    return;
+  }
+
   Offset = *InterruptContextOffset;
   Index = 0;
 
@@ -433,6 +447,17 @@ DumpIortNodeIts (
             PARSER_PARAMS (IortNodeItsParser)
             );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if (ItsCount == NULL) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient ITS group length. Length = %d.\n",
+      Length
+      );
+    return;
+  }
+
   Index = 0;
 
   while ((Index < *ItsCount) &&
@@ -617,6 +642,18 @@ ParseAcpiIort (
     PARSER_PARAMS (IortParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((IortNodeCount == NULL) ||
+      (IortNodeOffset == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   Offset = *IortNodeOffset;
   NodePtr = Ptr + Offset;
   Index = 0;
@@ -635,6 +672,21 @@ ParseAcpiIort (
       PARSER_PARAMS (IortNodeHeaderParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((IortNodeType == NULL)        ||
+        (IortNodeLength == NULL)      ||
+        (IortIdMappingCount == NULL)  ||
+        (IortIdMappingOffset == NULL)) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"IORT node header. Length = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure the IORT Node is inside the table
     if ((Offset + (*IortNodeLength)) > AcpiTableLength) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 10/11] ShellPkg: acpiview: GTDT: Validate global pointers before use

[Krzysztof Koch]

Check if global (in the scope of the GTDT parser) pointers have been
successfully updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 57174e14c80072f12b90e1996ebe8f0002d0c404..699a55b549ec3fa61bbd156898821055dc019199 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -189,6 +189,18 @@ DumpGTBlock (
     PARSER_PARAMS (GtBlockParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((GtBlockTimerCount == NULL) ||
+      (GtBlockTimerOffset == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient GT Block Structure length. Length = %d.\n",
+      Length
+      );
+    return;
+  }
+
   Offset = *GtBlockTimerOffset;
   Index = 0;
 
@@ -272,6 +284,18 @@ ParseAcpiGtdt (
     PARSER_PARAMS (GtdtParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((GtdtPlatformTimerCount == NULL) ||
+      (GtdtPlatformTimerOffset == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d.\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   TimerPtr = Ptr + *GtdtPlatformTimerOffset;
   Offset = *GtdtPlatformTimerOffset;
   Index = 0;
@@ -290,6 +314,19 @@ ParseAcpiGtdt (
       PARSER_PARAMS (GtPlatformTimerHeaderParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((PlatformTimerType == NULL) ||
+        (PlatformTimerLength == NULL)) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"Platform Timer Structure header. Length = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure the Platform Timer is inside the table.
     if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v3 11/11] ShellPkg: acpiview: DBG2: Validate global pointers before use

[Krzysztof Koch]

Check if global (in the scope of the DBG2 parser) pointers have been
successfully updated before they are used for further table parsing.

Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
---

Notes:
    v3:
    - Rebase on latest master [Krzysztof]

    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 43 ++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 869e700b9beda4886bf7bc5ae4ced3ab9a59efa3..0f730a306a94329a23fbaf54b59f1833b44616ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -123,6 +123,24 @@ DumpDbgDeviceInfo (
     PARSER_PARAMS (DbgDevInfoParser)
     );
 
+  // 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;
   Offset = *BaseAddrRegOffset;
@@ -224,6 +242,18 @@ ParseAcpiDbg2 (
              PARSER_PARAMS (Dbg2Parser)
              );
 
+  // 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;
 
@@ -239,6 +269,19 @@ ParseAcpiDbg2 (
       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 ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v3 00/11] Test against invalid pointers in acpiview

[Gao, Zhichao]

Sorry for the misunderstanding before. The patch set is good to me.
Series: Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Monday, January 20, 2020 7:14 PM
> To: devel@edk2.groups.io
> Cc: 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 v3 00/11] Test against invalid pointers in acpiview
> 
> Prevent the use of invalid pointers when parsing ACPI tables in the UEFI shell
> acpiview tool.
> 
> The parsing of ACPI tables is often controlled with the values read earlier from
> the same table. For example, the 'Offset' or 'Count' fields found in a structure
> are later used to parse the substructures. If such fields lie outside the structure's
> buffer length provided, then there is a possibility for a wild or dangling pointer.
> 
> Currently, if the ParseAcpi() function terminates early because the end of the
> input table data buffer has been reached, then the pointers which were
> supposed to be updated by this function are left untouched.
> This is a security issue as the values pointed to by these pointers are later used
> for flow control.
> 
> This patch series aims to solve this security issue by explicitly initializing any
> pointers lying outside the input ACPI data buffer to NULL and testing for NULL
> whenever these pointers are dereferenced.
> 
> Changes can be seet at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_add_pointer_validation_v3
> 
> Notes:
>     v3:
>     - Rebase on latest master [Krzysztof]
> 
>     v2:
>     - Do not require FadtMinorRevision and X_DsdtAddress pointers to be
>       valid in FADT table parser [Zhichao]
> 
>     v1:
>     - Validate static pointers in acpiview parsers before use [Krzysztof]
> 
> Krzysztof Koch (11):
>   ShellPkg: acpiview: Set ItemPtr to NULL for unprocessed table fields
>   ShellPkg: acpiview: RSDP: Validate global pointer before use
>   ShellPkg: acpiview: FADT: Validate global pointer before use
>   ShellPkg: acpiview: SLIT: Validate global pointer before use
>   ShellPkg: acpiview: SLIT: Validate System Locality count
>   ShellPkg: acpiview: SRAT: Validate global pointers before use
>   ShellPkg: acpiview: MADT: Validate global pointers before use
>   ShellPkg: acpiview: PPTT: Validate global pointers before use
>   ShellPkg: acpiview: IORT: Validate global pointers before use
>   ShellPkg: acpiview: GTDT: Validate global pointers before use
>   ShellPkg: acpiview: DBG2: Validate global pointers before use
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              |  9 ++-
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |
> 43 ++++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 21
> +++----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 37
> ++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 52
> +++++++++++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c |
> 13 +++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 25
> ++++++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c |
> 12 ++++
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 61
> ++++++++++++++++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 13
> +++++
>  10 files changed, 269 insertions(+), 17 deletions(-)
> 
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>