[PATCH v1 00/11] Add security checks in the Acpiview table parsers

[PATCH v1 00/11] Add security checks in the Acpiview table parsers

[Krzysztof Koch]

The following patches modify existing ACPI table parsers to add checks which
prevent many potential security issues. These include:
1. Entering infinite loops when ACPI structure lengths are zero.
2. Use of pointers which failed to be initialized because of invalid ACPI
table/structure lengths.
3. Buffer overruns caused by structures which have a too large value of the
'Length' field given the size of the buffer in which they are located.

Other changes added in this patchset include:
1. Removal of redundant forward STATIC function declarations for reducing
the code size.
2. Extension of the use of the -q flag to make ACPI table content validation
optional. ACPI table content consistency checks which do not affect the flow
control in the parsing logic can now be disabled. The remaining validation
checks are enforced as they also prevent the security issues listed above.

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/tree/612_enhance_parser_logic_v1

Krzysztof Koch (11):
  ShellPkg: acpiview: FADT: Validate global pointers before use
  ShellPkg: acpiview: SPCR: Remove redundant forward declaration
  ShellPkg: acpiview: RSDP: Make printing table checksum optional
  ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call
  ShellPkg: acpiview: SLIT: Add error-checking in the parsing logic
  ShellPkg: acpiview: SRAT: Add error-checking in the parsing logic
  ShellPkg: acpiview: MADT: Add error-checking in the parsing logic
  ShellPkg: acpiview: PPTT: Add error-checking in the parsing logic
  ShellPkg: acpiview: IORT: Add error-checking in the parsing logic
  ShellPkg: acpiview: GTDT: Add error-checking in the parsing logic
  ShellPkg: acpiview: DBG2: Add error-checking in the parsing logic

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              |  26 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |   8 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c | 298 +++++++++-----
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 131 +++---
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 294 ++++++++------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 419 +++++++++++++-------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 187 ++++-----
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c |  95 ++++-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 144 ++++---
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 115 ++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c |  98 ++---
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 113 +++---
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c |  22 +-
 13 files changed, 1150 insertions(+), 800 deletions(-)

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

[PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use

[Krzysztof Koch]

1. Check if the global pointer have been successfully updated before
they are later used to control the parsing logic in the FADT acpiview
parser.

2. Remove redundant forward function declarations by repositioning
blocks of code.

3. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302590a7d31f080c3952

Notes:
    v1:
    - improve the logic in the parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 131 ++++++++------------
 1 file changed, 51 insertions(+), 80 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index cee7ee0770433da96d6042d2f5d687903f4b5495..600d3b16d7b22b61c1a1fd21ecb93f16c7f8fa1a 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 - 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):
@@ -12,6 +12,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local variables
 STATIC CONST UINT32* DsdtAddress;
@@ -46,7 +47,17 @@ EFIAPI
 ValidateFirmwareCtrl (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+)
+{
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  if (*(UINT32*)Ptr != 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: Firmware Control must be zero for ARM platforms."
+    );
+  }
+#endif
+}
 
 /**
   This function validates the X_Firmware Control Field.
@@ -61,7 +72,17 @@ EFIAPI
 ValidateXFirmwareCtrl (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+)
+{
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  if (*(UINT64*)Ptr != 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: X Firmware Control must be zero for ARM platforms."
+    );
+  }
+#endif
+}
 
 /**
   This function validates the flags.
@@ -76,7 +97,17 @@ EFIAPI
 ValidateFlags (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+)
+{
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
+    );
+  }
+#endif
+}
 
 /**
   An ACPI_PARSER array describing the ACPI FADT Table.
@@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
   {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL}
 };
 
-/**
-  This function validates the Firmware Control Field.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateFirmwareCtrl (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-)
-{
-#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  if (*(UINT32*)Ptr != 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: Firmware Control must be zero for ARM platforms."
-    );
-  }
-#endif
-}
-
-/**
-  This function validates the X_Firmware Control Field.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateXFirmwareCtrl (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-)
-{
-#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  if (*(UINT64*)Ptr != 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: X Firmware Control must be zero for ARM platforms."
-    );
-  }
-#endif
-}
-
-/**
-  This function validates the flags.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateFlags (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-)
-{
-#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
-    );
-  }
-#endif
-}
-
 /**
   This function parses the ACPI FADT table.
   This function parses the FADT table and optionally traces the ACPI table fields.
@@ -248,12 +204,27 @@ ParseAcpiFadt (
     PARSER_PARAMS (FadtParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((DsdtAddress == NULL)       ||
+      (FadtMinorRevision == NULL) ||
+      (X_DsdtAddress == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d. " \
+        L"FADT parsing aborted.\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   if (Trace) {
     Print (L"\nSummary:\n");
     PrintFieldName (2, L"FADT Version");
     Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
 
-    if (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId) {
+    if (GetConsistencyChecking () &&
+        (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId)) {
       IncrementErrorCount ();
       Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n");
     }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use

[Carsey, Jaben]

I think it would be easier to see/review these changes logically if the functional changes (1 and 3) were separate from the refactoring change (2).

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, July 11, 2019 11: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 01/11] ShellPkg: acpiview: FADT: Validate global pointers
> before use
> 
> 1. Check if the global pointer have been successfully updated before
> they are later used to control the parsing logic in the FADT acpiview
> parser.
> 
> 2. Remove redundant forward function declarations by repositioning
> blocks of code.
> 
> 3. Allow silencing ACPI table content validation errors which do not
> cause table parsing to fail.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302
> 590a7d31f080c3952
> 
> Notes:
>     v1:
>     - improve the logic in the parser [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> | 131 ++++++++------------
>  1 file changed, 51 insertions(+), 80 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> index
> cee7ee0770433da96d6042d2f5d687903f4b5495..600d3b16d7b22b61c1a1fd21
> ecb93f16c7f8fa1a 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 - 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):
> @@ -12,6 +12,7 @@
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
> +#include "AcpiView.h"
> 
>  // Local variables
>  STATIC CONST UINT32* DsdtAddress;
> @@ -46,7 +47,17 @@ EFIAPI
>  ValidateFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT32*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    This function validates the X_Firmware Control Field.
> @@ -61,7 +72,17 @@ EFIAPI
>  ValidateXFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT64*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    This function validates the flags.
> @@ -76,7 +97,17 @@ EFIAPI
>  ValidateFlags (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    An ACPI_PARSER array describing the ACPI FADT Table.
> @@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
>    {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL}
>  };
> 
> -/**
> -  This function validates the Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT32*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the X_Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateXFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT64*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the flags.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFlags (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
>  /**
>    This function parses the ACPI FADT table.
>    This function parses the FADT table and optionally traces the ACPI table
> fields.
> @@ -248,12 +204,27 @@ ParseAcpiFadt (
>      PARSER_PARAMS (FadtParser)
>      );
> 
> +  // Check if the values used to control the parsing logic have been
> +  // successfully read.
> +  if ((DsdtAddress == NULL)       ||
> +      (FadtMinorRevision == NULL) ||
> +      (X_DsdtAddress == NULL)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Insufficient table length. AcpiTableLength = %d. " \
> +        L"FADT parsing aborted.\n",
> +      AcpiTableLength
> +      );
> +    return;
> +  }
> +
>    if (Trace) {
>      Print (L"\nSummary:\n");
>      PrintFieldName (2, L"FADT Version");
>      Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
> 
> -    if (*GetAcpiXsdtHeaderInfo ()->OemTableId !=
> *AcpiHdrInfo.OemTableId) {
> +    if (GetConsistencyChecking () &&
> +        (*GetAcpiXsdtHeaderInfo ()->OemTableId !=
> *AcpiHdrInfo.OemTableId)) {
>        IncrementErrorCount ();
>        Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n");
>      }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use

[Krzysztof Koch]

Hi Jaben,

I will split the changes into separate patch sets with each patch set having the same logical change made to every applicable acpiview table parser. The per-parser modifications will be separate commits as well.

Kind regards,

Krzysztof

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Carsey, Jaben via Groups.Io
Sent: Friday, July 12, 2019 15:27
To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use

I think it would be easier to see/review these changes logically if the functional changes (1 and 3) were separate from the refactoring change (2).

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, July 11, 2019 11: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 01/11] ShellPkg: acpiview: FADT: Validate global
> pointers before use
>
> 1. Check if the global pointer have been successfully updated before
> they are later used to control the parsing logic in the FADT acpiview
> parser.
>
> 2. Remove redundant forward function declarations by repositioning
> blocks of code.
>
> 3. Allow silencing ACPI table content validation errors which do not
> cause table parsing to fail.
>
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
>
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302
> 590a7d31f080c3952
>
> Notes:
>     v1:
>     - improve the logic in the parser [Krzysztof]
>
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> | 131 ++++++++------------
>  1 file changed, 51 insertions(+), 80 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> index
> cee7ee0770433da96d6042d2f5d687903f4b5495..600d3b16d7b22b61c1a1fd21
> ecb93f16c7f8fa1a 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 - 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):
> @@ -12,6 +12,7 @@
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
> +#include "AcpiView.h"
>
>  // Local variables
>  STATIC CONST UINT32* DsdtAddress;
> @@ -46,7 +47,17 @@ EFIAPI
>  ValidateFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT32*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
>
>  /**
>    This function validates the X_Firmware Control Field.
> @@ -61,7 +72,17 @@ EFIAPI
>  ValidateXFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT64*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
>
>  /**
>    This function validates the flags.
> @@ -76,7 +97,17 @@ EFIAPI
>  ValidateFlags (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> +    );
> +  }
> +#endif
> +}
>
>  /**
>    An ACPI_PARSER array describing the ACPI FADT Table.
> @@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
>    {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL,
> NULL}  };
>
> -/**
> -  This function validates the Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT32*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the X_Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateXFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT64*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the flags.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFlags (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
>  /**
>    This function parses the ACPI FADT table.
>    This function parses the FADT table and optionally traces the ACPI
> table fields.
> @@ -248,12 +204,27 @@ ParseAcpiFadt (
>      PARSER_PARAMS (FadtParser)
>      );
>
> +  // Check if the values used to control the parsing logic have been
> + // successfully read.
> +  if ((DsdtAddress == NULL)       ||
> +      (FadtMinorRevision == NULL) ||
> +      (X_DsdtAddress == NULL)) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Insufficient table length. AcpiTableLength = %d. " \
> +        L"FADT parsing aborted.\n",
> +      AcpiTableLength
> +      );
> +    return;
> +  }
> +
>    if (Trace) {
>      Print (L"\nSummary:\n");
>      PrintFieldName (2, L"FADT Version");
>      Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
>
> -    if (*GetAcpiXsdtHeaderInfo ()->OemTableId !=
> *AcpiHdrInfo.OemTableId) {
> +    if (GetConsistencyChecking () &&
> +        (*GetAcpiXsdtHeaderInfo ()->OemTableId !=
> *AcpiHdrInfo.OemTableId)) {
>        IncrementErrorCount ();
>        Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n");
>      }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>




IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use

[Gao, Zhichao]

Sorry for late review.

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> 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: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate
> global pointers before use
> 
> 1. Check if the global pointer have been successfully updated before they are
> later used to control the parsing logic in the FADT acpiview parser.
> 
> 2. Remove redundant forward function declarations by repositioning blocks
> of code.
> 
> 3. Allow silencing ACPI table content validation errors which do not cause
> table parsing to fail.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302
> 590a7d31f080c3952
> 
> Notes:
>     v1:
>     - improve the logic in the parser [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> | 131 ++++++++------------
>  1 file changed, 51 insertions(+), 80 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> index
> cee7ee0770433da96d6042d2f5d687903f4b5495..600d3b16d7b22b61c1a1fd21
> ecb93f16c7f8fa1a 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtPars
> +++ er.c
> @@ -1,7 +1,7 @@
>  /** @file
>    FADT 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):
> @@ -12,6 +12,7 @@
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
> +#include "AcpiView.h"
> 
>  // Local variables
>  STATIC CONST UINT32* DsdtAddress;
> @@ -46,7 +47,17 @@ EFIAPI
>  ValidateFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT32*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    This function validates the X_Firmware Control Field.
> @@ -61,7 +72,17 @@ EFIAPI
>  ValidateXFirmwareCtrl (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (*(UINT64*)Ptr != 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    This function validates the flags.
> @@ -76,7 +97,17 @@ EFIAPI
>  ValidateFlags (
>    IN UINT8* Ptr,
>    IN VOID*  Context
> -  );
> +)
> +{
> +#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> +  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> +    );
> +  }
> +#endif
> +}
> 
>  /**
>    An ACPI_PARSER array describing the ACPI FADT Table.
> @@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] = {
>    {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL}  };
> 
> -/**
> -  This function validates the Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT32*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the X_Firmware Control Field.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateXFirmwareCtrl (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (*(UINT64*)Ptr != 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: X Firmware Control must be zero for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
> -/**
> -  This function validates the flags.
> -
> -  @param [in] Ptr     Pointer to the start of the field data.
> -  @param [in] Context Pointer to context specific information e.g. this
> -                      could be a pointer to the ACPI table header.
> -**/
> -STATIC
> -VOID
> -EFIAPI
> -ValidateFlags (
> -  IN UINT8* Ptr,
> -  IN VOID*  Context
> -)
> -{
> -#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
> -  if (((*(UINT32*)Ptr) & HW_REDUCED_ACPI) == 0) {
> -    IncrementErrorCount ();
> -    Print (
> -      L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms."
> -    );
> -  }
> -#endif
> -}
> -
>  /**
>    This function parses the ACPI FADT table.
>    This function parses the FADT table and optionally traces the ACPI table
> fields.
> @@ -248,12 +204,27 @@ ParseAcpiFadt (
>      PARSER_PARAMS (FadtParser)
>      );
> 
> +  // Check if the values used to control the parsing logic have been
> + // successfully read.
> +  if ((DsdtAddress == NULL)       ||
> +      (FadtMinorRevision == NULL) ||
> +      (X_DsdtAddress == NULL)) {

FadtMinorRevision and X_DsdtAddress should not lead a error. The platform which is only implement acpi 1.0 doesn't have these two field. As an example, the OVMF with QEMU doesn't have these two field.
But the check is needed before using them.

Thanks,
Zhichao

> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Insufficient table length. AcpiTableLength = %d. " \
> +        L"FADT parsing aborted.\n",
> +      AcpiTableLength
> +      );
> +    return;
> +  }
> +
>    if (Trace) {
>      Print (L"\nSummary:\n");
>      PrintFieldName (2, L"FADT Version");
>      Print (L"%d.%d\n",  *AcpiHdrInfo.Revision, *FadtMinorRevision);
> 
> -    if (*GetAcpiXsdtHeaderInfo ()->OemTableId != *AcpiHdrInfo.OemTableId)
> {
> +    if (GetConsistencyChecking () &&
> +        (*GetAcpiXsdtHeaderInfo ()->OemTableId !=
> + *AcpiHdrInfo.OemTableId)) {
>        IncrementErrorCount ();
>        Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n");
>      }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
> 

[PATCH v1 02/11] ShellPkg: acpiview: SPCR: Remove redundant forward declaration

[Krzysztof Koch]

Reposition blocks of code to remove redundant forward function
declarations in order to reduce the code size.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/9be55a64f804c3d99e7c692208c8086d5b9ca553

Notes:
    v1:
    - remove redundant forward declarations [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c | 98 +++++++-------------
 1 file changed, 34 insertions(+), 64 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
index 1974a9c046e4a3bccccc55cf758184af097b2420..3b06b05dee8c056c6e009b9e485ccd35d4194e95 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Spcr/SpcrParser.c
@@ -1,7 +1,7 @@
 /** @file
   SPCR 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):
@@ -31,7 +31,23 @@ EFIAPI
 ValidateInterruptType (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  UINT8 InterruptType;
+
+  InterruptType = *Ptr;
+
+  if (InterruptType !=
+        EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERRUPT_TYPE_GIC) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: InterruptType = %d. This must be 8 on ARM Platforms",
+      InterruptType
+      );
+  }
+#endif
+}
 
 /**
   This function validates the Irq.
@@ -46,7 +62,22 @@ EFIAPI
 ValidateIrq (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  UINT8 Irq;
+
+  Irq = *Ptr;
+
+  if (Irq != 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: Irq = %d. This must be zero on ARM Platforms\n",
+      Irq
+      );
+  }
+#endif
+}
 
 /**
   An ACPI_PARSER array describing the ACPI SPCR Table.
@@ -76,67 +107,6 @@ STATIC CONST ACPI_PARSER SpcrParser[] = {
   {L"Reserved", 4, 76, L"%x", NULL, NULL, NULL, NULL}
 };
 
-/**
-  This function validates the Interrupt Type.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateInterruptType (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  UINT8 InterruptType;
-
-  InterruptType = *Ptr;
-
-  if (InterruptType !=
-        EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_INTERRUPT_TYPE_GIC) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: InterruptType = %d. This must be 8 on ARM Platforms",
-      InterruptType
-      );
-  }
-#endif
-}
-
-/**
-  This function validates the Irq.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateIrq (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-#if defined (MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  UINT8 Irq;
-
-  Irq = *Ptr;
-
-  if (Irq != 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: Irq = %d. This must be zero on ARM Platforms\n",
-      Irq
-      );
-  }
-#endif
-}
-
 /**
   This function parses the ACPI SPCR table.
   When trace is enabled this function parses the SPCR table and
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 02/11] ShellPkg: acpiview: SPCR: Remove redundant forward declaration

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

[PATCH v1 03/11] ShellPkg: acpiview: RSDP: Make printing table checksum optional

[Krzysztof Koch]

1. Don't validate Root System Description Pointer (RSDP) structure
checksum if the '-q' command line flag is used with the acpiview UEFI
shell tool. This change makes the RSDP parser consistent with the
parsers for other ACPI tables.

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

3. Remove redundant forward function declarations by repositioning
blocks of code.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/73e6d7e117da244f8f4065620115a47f7f66d372

Notes:
    v1:
    - make RSDP parser behavior consistent with other tables [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 144 +++++++++-----------
 1 file changed, 68 insertions(+), 76 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index 586de7cbfb12f856c0c735b6e295c1cc32eb2ceb..952517cd09aaff601bb363fd73331c750a9e97ff 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -1,7 +1,7 @@
 /** @file
   RSDP 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):
@@ -11,6 +11,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local Variables
 STATIC CONST UINT64* XsdtAddress;
@@ -28,7 +29,27 @@ EFIAPI
 ValidateRsdtAddress (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  // Reference: Server Base Boot Requirements System Software on ARM Platforms
+  // Section: 4.2.1.1 RSDP
+  // Root System Description Pointer (RSDP), ACPI ? 5.2.5.
+  //   - Within the RSDP, the RsdtAddress field must be null (zero) and the
+  //     XsdtAddresss MUST be a valid, non-null, 64-bit value.
+  UINT32 RsdtAddr;
+
+  RsdtAddr = *(UINT32*)Ptr;
+
+  if (RsdtAddr != 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: Rsdt Address = 0x%p. This must be NULL on ARM Platforms.",
+      RsdtAddr
+      );
+  }
+#endif
+}
 
 /**
   This function validates the XSDT Address.
@@ -43,7 +64,27 @@ EFIAPI
 ValidateXsdtAddress (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
+  // Reference: Server Base Boot Requirements System Software on ARM Platforms
+  // Section: 4.2.1.1 RSDP
+  // Root System Description Pointer (RSDP), ACPI ? 5.2.5.
+  //   - Within the RSDP, the RsdtAddress field must be null (zero) and the
+  //     XsdtAddresss MUST be a valid, non-null, 64-bit value.
+  UINT64 XsdtAddr;
+
+  XsdtAddr = *(UINT64*)Ptr;
+
+  if (XsdtAddr == 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: Xsdt Address = 0x%p. This must not be NULL on ARM Platforms.",
+      XsdtAddr
+      );
+  }
+#endif
+}
 
 /**
   An array describing the ACPI RSDP Table.
@@ -61,76 +102,6 @@ STATIC CONST ACPI_PARSER RsdpParser[] = {
   {L"Reserved", 3, 33, L"%x %x %x", Dump3Chars, NULL, NULL, NULL}
 };
 
-/**
-  This function validates the RSDT Address.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateRsdtAddress (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  // Reference: Server Base Boot Requirements System Software on ARM Platforms
-  // Section: 4.2.1.1 RSDP
-  // Root System Description Pointer (RSDP), ACPI ? 5.2.5.
-  //   - Within the RSDP, the RsdtAddress field must be null (zero) and the
-  //     XsdtAddresss MUST be a valid, non-null, 64-bit value.
-  UINT32 RsdtAddr;
-
-  RsdtAddr = *(UINT32*)Ptr;
-
-  if (RsdtAddr != 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: Rsdt Address = 0x%p. This must be NULL on ARM Platforms.",
-      RsdtAddr
-      );
-  }
-#endif
-}
-
-/**
-  This function validates the XSDT Address.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateXsdtAddress (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-#if defined(MDE_CPU_ARM) || defined (MDE_CPU_AARCH64)
-  // Reference: Server Base Boot Requirements System Software on ARM Platforms
-  // Section: 4.2.1.1 RSDP
-  // Root System Description Pointer (RSDP), ACPI ? 5.2.5.
-  //   - Within the RSDP, the RsdtAddress field must be null (zero) and the
-  //     XsdtAddresss MUST be a valid, non-null, 64-bit value.
-  UINT64 XsdtAddr;
-
-  XsdtAddr = *(UINT64*)Ptr;
-
-  if (XsdtAddr == 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: Xsdt Address = 0x%p. This must not be NULL on ARM Platforms.",
-      XsdtAddr
-      );
-  }
-#endif
-}
-
 /**
   This function parses the ACPI RSDP table.
 
@@ -156,10 +127,31 @@ ParseAcpiRsdp (
 {
   if (Trace) {
     DumpRaw (Ptr, AcpiTableLength);
-    VerifyChecksum (TRUE, Ptr, AcpiTableLength);
+    if (GetConsistencyChecking ()) {
+      VerifyChecksum (TRUE, Ptr, AcpiTableLength);
+    }
   }
 
-  ParseAcpi (Trace, 0, "RSDP", Ptr, AcpiTableLength, PARSER_PARAMS (RsdpParser));
+  ParseAcpi (
+    Trace,
+    0,
+    "RSDP",
+    Ptr,
+    AcpiTableLength,
+    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
@@ -167,7 +159,7 @@ ParseAcpiRsdp (
   // Therefore the RSDT should not be used on ARM platforms.
   if ((*XsdtAddress) == 0) {
     IncrementErrorCount ();
-    Print (L"ERROR: XSDT Pointer is not set.\n");
+    Print (L"ERROR: XSDT Pointer is not set. RSDP parsing aborted.\n");
     return;
   }
 
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 03/11] ShellPkg: acpiview: RSDP: Make printing table checksum optional

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

[PATCH v1 04/11] ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call

[Krzysztof Koch]

1. Remove a call to ParseAcpi responsible for getting the XSDT table
length. This call is not needed because the ACPI table buffer length is
provided as an input argument to the ParseAcpiXsdt() function.

2. Use ParseAcpiXsdt function argument instead of a global variable to
check against table buffer overruns.

3. Allow suppressing errors about invalid ACPI table poiners inside the
XSDT table.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/27b4ae23c4f230225d6bcb27598b42edcf329512

Notes:
    v1:
    - Remove a redundant call to ParseAcpi() [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c | 22 +++++++-------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
index 4196168bff47d70c67f79f3fc1f4cdee302d460e..b7d8f4215a71ef429fb88d6f2d998d8f38250716 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
@@ -1,7 +1,7 @@
 /** @file
   XSDT 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):
@@ -13,6 +13,7 @@
 #include <Library/PrintLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local variables
 STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
@@ -60,22 +61,12 @@ ParseAcpiXsdt (
   UINTN         EntryIndex;
   CHAR16        Buffer[32];
 
-  // Parse the ACPI header to get the length
-  ParseAcpi (
-    FALSE,
-    0,
-    "XSDT",
-    Ptr,
-    ACPI_DESCRIPTION_HEADER_LENGTH,
-    PARSER_PARAMS (XsdtParser)
-    );
-
   Offset = ParseAcpi (
              Trace,
              0,
              "XSDT",
              Ptr,
-             *AcpiHdrInfo.Length,
+             AcpiTableLength,
              PARSER_PARAMS (XsdtParser)
              );
 
@@ -84,7 +75,7 @@ ParseAcpiXsdt (
   if (Trace) {
     EntryIndex = 0;
     TablePointer = (UINT64*)(Ptr + TableOffset);
-    while (Offset < (*AcpiHdrInfo.Length)) {
+    while (Offset < AcpiTableLength) {
       CONST UINT32* Signature;
       CONST UINT32* Length;
       CONST UINT8*  Revision;
@@ -124,7 +115,8 @@ ParseAcpiXsdt (
       Print (L"0x%lx\n", *TablePointer);
 
       // Validate the table pointers are not NULL
-      if ((UINT64*)(UINTN)(*TablePointer) == NULL) {
+      if (GetConsistencyChecking () &&
+          ((UINT64*)(UINTN)(*TablePointer) == NULL)) {
         IncrementErrorCount ();
         Print (
           L"ERROR: Invalid table entry at 0x%lx, table address is 0x%lx\n",
@@ -140,7 +132,7 @@ ParseAcpiXsdt (
   // Process the tables
   Offset = TableOffset;
   TablePointer = (UINT64*)(Ptr + TableOffset);
-  while (Offset < (*AcpiHdrInfo.Length)) {
+  while (Offset < AcpiTableLength) {
     if ((UINT64*)(UINTN)(*TablePointer) != NULL) {
       ProcessAcpiTable ((UINT8*)(UINTN)(*TablePointer));
     }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 04/11] ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Test against buffer overruns.

3. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

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

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/2f1ffd1a74d06c23c01971be965d856f0a0e3ac4

Notes:
    v1:
    - improve the logic in the SLIT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c | 115 ++++++++++++++------
 1 file changed, 83 insertions(+), 32 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
index 1f9dac66eea5b0f4366a7a9584ac6702a74beaac..dd5f039b67326acc710ee703a6b132a1e280dcaa 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):
@@ -13,6 +13,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local Variables
 STATIC CONST UINT64* SlitSystemLocalityCount;
@@ -59,7 +60,7 @@ ParseAcpiSlit (
   UINT32 Offset;
   UINT64 Count;
   UINT64 Index;
-  UINT64 LocalityCount;
+  UINT32 LocalityCount;
   UINT8* LocalityPtr;
   CHAR16 Buffer[80];  // Used for AsciiName param of ParseAcpi
 
@@ -77,7 +78,55 @@ ParseAcpiSlit (
              );
   LocalityPtr = Ptr + Offset;
 
-  LocalityCount = *SlitSystemLocalityCount;
+  // 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;
+  }
+
+  /*
+    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;
+  }
+
   // 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.
@@ -96,7 +145,7 @@ ParseAcpiSlit (
       Print (L" (%3d) ", Index);
     }
     Print (L"\n");
-    for (Count = 0; Count< LocalityCount; Count++) {
+    for (Count = 0; Count < LocalityCount; Count++) {
       Print (L" (%3d) ", Count);
       for (Index = 0; Index < LocalityCount; Index++) {
         Print (L"  %3d  ", SLIT_ELEMENT (LocalityPtr, Count, Index));
@@ -106,34 +155,36 @@ ParseAcpiSlit (
   }
 
   // Validate
-  for (Count = 0; Count < LocalityCount; Count++) {
-    for (Index = 0; Index < LocalityCount; Index++) {
-      // Element[x][x] must be equal to 10
-      if ((Count == Index) && (SLIT_ELEMENT (LocalityPtr, Count,Index) != 10)) {
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Diagonal Element[0x%lx][0x%lx] (%3d)."
-            L" Normalized Value is not 10\n",
-          Count,
-          Index,
-          SLIT_ELEMENT (LocalityPtr, Count, Index)
-          );
-      }
-      // Element[i][j] must be equal to Element[j][i]
-      if (SLIT_ELEMENT (LocalityPtr, Count, Index) !=
-          SLIT_ELEMENT (LocalityPtr, Index, Count)) {
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Relative distances for Element[0x%lx][0x%lx] (%3d) and \n"
-           L"Element[0x%lx][0x%lx] (%3d) do not match.\n",
-          Count,
-          Index,
-          SLIT_ELEMENT (LocalityPtr, Count, Index),
-          Index,
-          Count,
-          SLIT_ELEMENT (LocalityPtr, Index, Count)
-          );
+  if (GetConsistencyChecking ()) {
+    for (Count = 0; Count < LocalityCount; Count++) {
+      for (Index = 0; Index < LocalityCount; Index++) {
+        // Element[x][x] must be equal to 10
+        if ((Count == Index) && (SLIT_ELEMENT (LocalityPtr, Count,Index) != 10)) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Diagonal Element[0x%lx][0x%lx] (%3d)."
+              L" Normalized Value is not 10\n",
+            Count,
+            Index,
+            SLIT_ELEMENT (LocalityPtr, Count, Index)
+            );
+        }
+        // Element[i][j] must be equal to Element[j][i]
+        if (SLIT_ELEMENT (LocalityPtr, Count, Index) !=
+            SLIT_ELEMENT (LocalityPtr, Index, Count)) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Relative distances for Element[0x%lx][0x%lx] (%3d) and \n"
+            L"Element[0x%lx][0x%lx] (%3d) do not match.\n",
+            Count,
+            Index,
+            SLIT_ELEMENT (LocalityPtr, Count, Index),
+            Index,
+            Count,
+            SLIT_ELEMENT (LocalityPtr, Index, Count)
+            );
+        }
       }
     }
-  }
+  } // if (GetConsistencyChecking ())
 }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Give forward progress guarantee when parsing the SRAT table. Report
an error if a SRAT structure is too small to be valid. Without this
check, there is a possibility for the parser to enter an ifninite loop.

3. Test against buffer overruns.

4. Remove redundant forward function declarations by repositioning
blocks of code.

5. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

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

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

Notes:
    v1:
    - improve the logic in the SRAT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 113 +++++++++++---------
 1 file changed, 63 insertions(+), 50 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 075ff2a141a82b522e8aaedb7ad79249aaf5eaac..a12aceb70d273a628387b72437819dc05ad7301e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -1,7 +1,7 @@
 /** @file
   SRAT 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):
@@ -13,6 +13,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local Variables
 STATIC CONST UINT8* SratRAType;
@@ -32,7 +33,13 @@ EFIAPI
 ValidateSratReserved (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  if (*(UINT32*)Ptr != 1) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: Reserved should be 1 for backward compatibility.\n");
+  }
+}
 
 /**
   This function traces the APIC Proximity Domain field.
@@ -44,9 +51,16 @@ STATIC
 VOID
 EFIAPI
 DumpSratApicProximity (
-  IN  CONST CHAR16*  Format,
-  IN  UINT8*         Ptr
-  );
+ IN CONST CHAR16* Format,
+ IN UINT8*        Ptr
+ )
+{
+  UINT32 ProximityDomain;
+
+  ProximityDomain = Ptr[0] | (Ptr[1] << 8) | (Ptr[2] << 16);
+
+  Print (Format, ProximityDomain);
+}
 
 /**
   An ACPI_PARSER array describing the SRAT Table.
@@ -139,47 +153,6 @@ STATIC CONST ACPI_PARSER SratX2ApciAffinityParser[] = {
   {L"Reserved", 4, 20, L"0x%x", NULL, NULL, NULL, NULL}
 };
 
-/** This function validates the Reserved field in the SRAT table header.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateSratReserved (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  if (*(UINT32*)Ptr != 1) {
-    IncrementErrorCount ();
-    Print (L"\nERROR: Reserved should be 1 for backward compatibility.\n");
-  }
-}
-
-/**
-  This function traces the APIC Proximity Domain field.
-
-  @param [in] Format  Format string for tracing the data.
-  @param [in] Ptr     Pointer to the start of the buffer.
-**/
-STATIC
-VOID
-EFIAPI
-DumpSratApicProximity (
- IN CONST CHAR16* Format,
- IN UINT8*        Ptr
- )
-{
-  UINT32 ProximityDomain;
-
-  ProximityDomain = Ptr[0] | (Ptr[1] << 8) | (Ptr[2] << 16);
-
-  Print (Format, ProximityDomain);
-}
-
 /**
   This function parses the ACPI SRAT table.
   When trace is enabled this function parses the SRAT table and
@@ -234,6 +207,7 @@ ParseAcpiSrat (
              AcpiTableLength,
              PARSER_PARAMS (SratParser)
              );
+
   ResourcePtr = Ptr + Offset;
 
   while (Offset < AcpiTableLength) {
@@ -242,10 +216,47 @@ ParseAcpiSrat (
       0,
       NULL,
       ResourcePtr,
-      2,  // The length is 1 byte at offset 1
+      AcpiTableLength - Offset,
       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 forward progress is made.
+    if (*SratRALength < 2) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Structure length is too small: SratRALength = %d. " \
+          L"SratRAType = %d. SRAT parsing aborted.\n",
+        *SratRALength,
+        *SratRAType
+        );
+      return;
+    }
+
+    // Make sure the SRAT structure lies inside the table
+    if ((Offset + *SratRALength) > AcpiTableLength) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid SRAT structure length. SratRALength = %d. " \
+          L"RemainingTableBufferLength = %d. SRAT parsing aborted.\n",
+        *SratRALength,
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     switch (*SratRAType) {
       case EFI_ACPI_6_2_GICC_AFFINITY:
         AsciiSPrint (
@@ -278,7 +289,7 @@ ParseAcpiSrat (
           ResourcePtr,
           *SratRALength,
           PARSER_PARAMS (SratGicITSAffinityParser)
-        );
+          );
         break;
 
       case EFI_ACPI_6_2_MEMORY_AFFINITY:
@@ -333,8 +344,10 @@ ParseAcpiSrat (
         break;
 
       default:
-        IncrementErrorCount ();
-        Print (L"ERROR: Unknown SRAT Affinity type = 0x%x\n", *SratRAType);
+        if (GetConsistencyChecking ()) {
+          IncrementErrorCount ();
+          Print (L"ERROR: Unknown SRAT Affinity type = 0x%x\n", *SratRAType);
+        }
         break;
     }
 
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Give forward progress guarantee when parsing the MADT table. Report
an error if a MADT structure is too small to be valid. Without this
check, there is a possibility for the parser to enter an ifninite loop.

3. Test against buffer overruns.

4. Remove redundant forward function declarations by repositioning
blocks of code.

5. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

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

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

Notes:
    v1:
    - improve the logic in the MADT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 187 ++++++++++----------
 1 file changed, 94 insertions(+), 93 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 59c3df0cc8a080497b517baf36fc63f1e4ab866f..54f9fddc5426de5383b747ec7afd21396bcccfc9 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -15,6 +15,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 #include "MadtParser.h"
 
 // Local Variables
@@ -35,7 +36,15 @@ EFIAPI
 ValidateGICDSystemVectorBase (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+)
+{
+  if (*(UINT32*)Ptr != 0) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: System Vector Base must be zero."
+    );
+  }
+}
 
 /**
   This function validates the SPE Overflow Interrupt in the GICC.
@@ -50,7 +59,41 @@ EFIAPI
 ValidateSpeOverflowInterrupt (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  UINT16 SpeOverflowInterrupt;
+
+  SpeOverflowInterrupt = *(UINT16*)Ptr;
+
+  // SPE not supported by this processor
+  if (SpeOverflowInterrupt == 0) {
+    return;
+  }
+
+  if ((SpeOverflowInterrupt < ARM_PPI_ID_MIN) ||
+      ((SpeOverflowInterrupt > ARM_PPI_ID_MAX) &&
+       (SpeOverflowInterrupt < ARM_PPI_ID_EXTENDED_MIN)) ||
+      (SpeOverflowInterrupt > ARM_PPI_ID_EXTENDED_MAX)) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: SPE Overflow Interrupt ID of %d is not in the allowed PPI ID "
+        L"ranges of %d-%d or %d-%d (for GICv3.1 or later).",
+      SpeOverflowInterrupt,
+      ARM_PPI_ID_MIN,
+      ARM_PPI_ID_MAX,
+      ARM_PPI_ID_EXTENDED_MIN,
+      ARM_PPI_ID_EXTENDED_MAX
+    );
+  } else if (SpeOverflowInterrupt != ARM_PPI_ID_PMBIRQ) {
+    IncrementWarningCount();
+    Print (
+      L"\nWARNING: SPE Overflow Interrupt ID of %d is not compliant with SBSA "
+        L"Level 3 PPI ID assignment: %d.",
+      SpeOverflowInterrupt,
+      ARM_PPI_ID_PMBIRQ
+    );
+  }
+}
 
 /**
   An ACPI_PARSER array describing the GICC Interrupt Controller Structure.
@@ -158,78 +201,6 @@ STATIC CONST ACPI_PARSER MadtInterruptControllerHeaderParser[] = {
   {L"Reserved", 2, 2, NULL, NULL, NULL, NULL, NULL}
 };
 
-/**
-  This function validates the System Vector Base in the GICD.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateGICDSystemVectorBase (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-)
-{
-  if (*(UINT32*)Ptr != 0) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: System Vector Base must be zero."
-    );
-  }
-}
-
-/**
-  This function validates the SPE Overflow Interrupt in the GICC.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateSpeOverflowInterrupt (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  UINT16 SpeOverflowInterrupt;
-
-  SpeOverflowInterrupt = *(UINT16*)Ptr;
-
-  // SPE not supported by this processor
-  if (SpeOverflowInterrupt == 0) {
-    return;
-  }
-
-  if ((SpeOverflowInterrupt < ARM_PPI_ID_MIN) ||
-      ((SpeOverflowInterrupt > ARM_PPI_ID_MAX) &&
-       (SpeOverflowInterrupt < ARM_PPI_ID_EXTENDED_MIN)) ||
-      (SpeOverflowInterrupt > ARM_PPI_ID_EXTENDED_MAX)) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: SPE Overflow Interrupt ID of %d is not in the allowed PPI ID "
-        L"ranges of %d-%d or %d-%d (for GICv3.1 or later).",
-      SpeOverflowInterrupt,
-      ARM_PPI_ID_MIN,
-      ARM_PPI_ID_MAX,
-      ARM_PPI_ID_EXTENDED_MIN,
-      ARM_PPI_ID_EXTENDED_MAX
-    );
-  } else if (SpeOverflowInterrupt != ARM_PPI_ID_PMBIRQ) {
-    IncrementWarningCount();
-    Print (
-      L"\nWARNING: SPE Overflow Interrupt ID of %d is not compliant with SBSA "
-        L"Level 3 PPI ID assignment: %d.",
-      SpeOverflowInterrupt,
-      ARM_PPI_ID_PMBIRQ
-    );
-  }
-}
-
 /**
   This function parses the ACPI MADT table.
   When trace is enabled this function parses the MADT table and
@@ -286,20 +257,47 @@ ParseAcpiMadt (
       0,
       NULL,
       InterruptContollerPtr,
-      2,  //  Length is 1 byte at offset 1
+      AcpiTableLength - Offset,
       PARSER_PARAMS (MadtInterruptControllerHeaderParser)
       );
 
-    if (((Offset + (*MadtInterruptControllerLength)) > AcpiTableLength) ||
-        (*MadtInterruptControllerLength < 4)) {
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((MadtInterruptControllerType == NULL) ||
+        (MadtInterruptControllerLength == NULL)) {
       IncrementErrorCount ();
       Print (
-         L"ERROR: Invalid Interrupt Controller Length,"
-          L" Type = %d, Length = %d\n",
-         *MadtInterruptControllerType,
-         *MadtInterruptControllerLength
-         );
-      break;
+        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 ();
+      Print (
+        L"ERROR: Structure length is too small: " \
+          L"MadtInterruptControllerLength = %d. " \
+          L"MadtInterruptControllerType = %d. MADT parsing aborted.\n",
+        *MadtInterruptControllerLength,
+        *MadtInterruptControllerType
+        );
+      return;
+    }
+
+    // Make sure the MADT structure lies inside the table
+    if ((Offset + *MadtInterruptControllerLength) > AcpiTableLength) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid MADT structure length. " \
+          L"MadtInterruptControllerLength = %d. " \
+          L"RemainingTableBufferLength = %d. MADT parsing aborted.\n",
+        *MadtInterruptControllerLength,
+        AcpiTableLength - Offset
+        );
+      return;
     }
 
     switch (*MadtInterruptControllerType) {
@@ -316,11 +314,11 @@ ParseAcpiMadt (
       }
 
       case EFI_ACPI_6_3_GICD: {
-        if (++GICDCount > 1) {
+        if (GetConsistencyChecking() &&
+            (++GICDCount > 1)) {
           IncrementErrorCount ();
           Print (
-            L"ERROR: Only one GICD must be present,"
-              L" GICDCount = %d\n",
+            L"ERROR: Only one GICD must be present. GICDCount = %d\n",
             GICDCount
             );
         }
@@ -372,13 +370,16 @@ ParseAcpiMadt (
       }
 
       default: {
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Unknown Interrupt Controller Structure,"
-            L" Type = %d, Length = %d\n",
-          *MadtInterruptControllerType,
-          *MadtInterruptControllerLength
-          );
+        if (GetConsistencyChecking ()) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Unknown Interrupt Controller Structure,"
+              L" Type = %d, Length = %d\n",
+            *MadtInterruptControllerType,
+            *MadtInterruptControllerLength
+            );
+        }
+        break;
       }
     } // switch
 
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Give forward progress guarantee when parsing the PPTT table. Report
an error if a PPTT structure is too small to be valid. Without this
check, there is a possibility for the parser to enter an ifninite loop.

3. Test against buffer overruns.

4. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

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

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

Notes:
    v1:
    - improve the logic in the PPTT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c | 95 ++++++++++++++++----
 1 file changed, 76 insertions(+), 19 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
index cec57be55e77096f9448f637ea129af2b42111ad..8d8760940b493eb94c91da3d46f9a844930c1738 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
@@ -252,7 +252,6 @@ DumpProcessorHierarchyNodeStructure (
   )
 {
   UINT32 Offset;
-  UINT8* PrivateResourcePtr;
   UINT32 Index;
   CHAR16 Buffer[OUTPUT_FIELD_COLUMN_WIDTH];
 
@@ -265,8 +264,34 @@ DumpProcessorHierarchyNodeStructure (
              PARSER_PARAMS (ProcessorHierarchyNodeStructureParser)
              );
 
-  PrivateResourcePtr = Ptr + Offset;
+  // 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 ();
+    Print (
+      L"ERROR: Invalid Number of Private Resources. " \
+        L"PrivateResourceCount = %d. RemainingBufferLength = %d. " \
+        L"Parsing of this structure aborted.\n",
+      *NumberOfPrivateResources,
+      Length - Offset
+      );
+    return;
+  }
+
   Index = 0;
+
+  // Parse the specified number of private resource references or the Processor
+  // Hierarchy Node length. Whichever is minimum.
   while (Index < *NumberOfPrivateResources) {
     UnicodeSPrint (
       Buffer,
@@ -278,10 +303,10 @@ DumpProcessorHierarchyNodeStructure (
     PrintFieldName (4, Buffer);
     Print (
       L"0x%x\n",
-      *((UINT32*) PrivateResourcePtr)
+      *((UINT32*)(Ptr + Offset))
       );
 
-    PrivateResourcePtr += sizeof(UINT32);
+    Offset += sizeof (UINT32);
     Index++;
   }
 }
@@ -373,6 +398,7 @@ ParseAcpiPptt (
              AcpiTableLength,
              PARSER_PARAMS (PpttParser)
              );
+
   ProcessorTopologyStructurePtr = Ptr + Offset;
 
   while (Offset < AcpiTableLength) {
@@ -382,19 +408,47 @@ ParseAcpiPptt (
       0,
       NULL,
       ProcessorTopologyStructurePtr,
-      4,  // Length of the processor topology structure header is 4 bytes
+      AcpiTableLength - Offset,
       PARSER_PARAMS (ProcessorTopologyStructureHeaderParser)
       );
 
-    if ((Offset + (*ProcessorTopologyStructureLength)) > AcpiTableLength) {
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if ((ProcessorTopologyStructureType == NULL) ||
+        (ProcessorTopologyStructureLength == NULL)) {
       IncrementErrorCount ();
       Print (
-        L"ERROR: Invalid processor topology structure length:"
-          L" Type = %d, Length = %d\n",
-        *ProcessorTopologyStructureType,
-        *ProcessorTopologyStructureLength
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"processor topology structure header. Length = %d.\n",
+        AcpiTableLength - Offset
         );
-      break;
+      return;
+    }
+
+    // Make sure forward progress is made.
+    if (*ProcessorTopologyStructureLength < 2) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Structure length is too small: " \
+          L"ProcessorTopologyStructureLength = %d. " \
+          L"ProcessorTopologyStructureType = %d. PPTT parsing aborted.\n",
+        *ProcessorTopologyStructureLength,
+        *ProcessorTopologyStructureType
+        );
+      return;
+    }
+
+    // Make sure the PPTT structure lies inside the table
+    if ((Offset + *ProcessorTopologyStructureLength) > AcpiTableLength) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid PPTT structure length. " \
+          L"ProcessorTopologyStructureLength = %d. " \
+          L"RemainingTableBufferLength = %d. PPTT parsing aborted.\n",
+        *ProcessorTopologyStructureLength,
+        AcpiTableLength - Offset
+        );
+      return;
     }
 
     PrintFieldName (2, L"* Structure Offset *");
@@ -420,14 +474,17 @@ ParseAcpiPptt (
           );
         break;
       default:
-        IncrementErrorCount ();
-        Print (
-          L"ERROR: Unknown processor topology structure:"
-            L" Type = %d, Length = %d\n",
-          *ProcessorTopologyStructureType,
-          *ProcessorTopologyStructureLength
-          );
-    }
+        if (GetConsistencyChecking ()) {
+          IncrementErrorCount ();
+          Print (
+            L"ERROR: Unknown processor topology structure:"
+              L" Type = %d, Length = %d\n",
+            *ProcessorTopologyStructureType,
+            *ProcessorTopologyStructureLength
+            );
+        }
+        break;
+    } // switch
 
     ProcessorTopologyStructurePtr += *ProcessorTopologyStructureLength;
     Offset += *ProcessorTopologyStructureLength;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

5. Move ID mapping count validation for the PMCG node to the
IortNodePmcgParser[] ACPI_PARSER array. This check does not affect
the flow of IORT parsing and is limited to a single table field in
scope.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/0b398f116f7aed99dbec4090b5c2c0ed93273ef7

Notes:
    v1:
    - improve the logic in the IORT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 419 +++++++++++++-------
 1 file changed, 279 insertions(+), 140 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 93f78e1a9786ed53f6b5529f478b72a220b4f8df..f09e7aeeb34bf4c3d9564240b53539c8d6811f66 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -13,6 +13,7 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
 
 // Local variables
 STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
@@ -45,7 +46,35 @@ EFIAPI
 ValidateItsIdMappingCount (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  if (*(UINT32*)Ptr != 0) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: IORT ID Mapping count must be zero.");
+  }
+}
+
+/**
+  This function validates the ID Mapping array count for the Performance
+  Monitoring Counter Group (PMCG) node.
+
+  @param [in] Ptr     Pointer to the start of the field data.
+  @param [in] Context Pointer to context specific information e.g. this
+                      could be a pointer to the ACPI table header.
+**/
+STATIC
+VOID
+EFIAPI
+ValidatePmcgIdMappingCount (
+  IN UINT8* Ptr,
+  IN VOID*  Context
+  )
+{
+  if (*(UINT32*)Ptr > 1) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: IORT ID Mapping count must not be greater than 1.");
+  }
+}
 
 /**
   This function validates the ID Mapping array offset for the ITS node.
@@ -60,7 +89,13 @@ EFIAPI
 ValidateItsIdArrayReference (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  if (*(UINT32*)Ptr != 0) {
+    IncrementErrorCount ();
+    Print (L"\nERROR: IORT ID Mapping offset must be zero.");
+  }
+}
 
 /**
   Helper Macro for populating the IORT Node header in the ACPI_PARSER array.
@@ -204,95 +239,65 @@ STATIC CONST ACPI_PARSER IortNodeRootComplexParser[] = {
   An ACPI_PARSER array describing the IORT PMCG node.
 **/
 STATIC CONST ACPI_PARSER IortNodePmcgParser[] = {
-  PARSE_IORT_NODE_HEADER (NULL, NULL),
+  PARSE_IORT_NODE_HEADER (ValidatePmcgIdMappingCount, NULL),
   {L"Base Address", 8, 16, L"0x%lx", NULL, NULL, NULL, NULL},
   {L"Overflow interrupt GSIV", 4, 24, L"0x%x", NULL, NULL, NULL, NULL},
   {L"Node reference", 4, 28, L"0x%x", NULL, NULL, NULL, NULL},
 };
 
-/**
-  This function validates the ID Mapping array count for the ITS node.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateItsIdMappingCount (
-  IN UINT8* Ptr,
-  IN VOID*     Context
-  )
-{
-  if (*(UINT32*)Ptr != 0) {
-    IncrementErrorCount ();
-    Print (L"\nERROR: IORT ID Mapping count must be zero.");
-  }
-}
-
-/**
-  This function validates the ID Mapping array offset for the ITS node.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateItsIdArrayReference (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  if (*(UINT32*)Ptr != 0) {
-    IncrementErrorCount ();
-    Print (L"\nERROR: IORT ID Mapping offset must be zero.");
-  }
-}
-
 /**
   This function parses the IORT Node Id Mapping array.
 
-  @param [in] Ptr            Pointer to the start of the IORT Table.
+  @param [in] Ptr            Pointer to the start of the ID mapping array.
+  @param [in] Length         Length of the buffer.
   @param [in] MappingCount   The ID Mapping count.
-  @param [in] MappingOffset  The offset of the ID Mapping array
-                             from the start of the IORT table.
 **/
 STATIC
 VOID
 DumpIortNodeIdMappings (
   IN UINT8* Ptr,
-  IN UINT32 MappingCount,
-  IN UINT32 MappingOffset
+  IN UINT32 Length,
+  IN UINT32 MappingCount
   )
 {
-  UINT8* IdMappingPtr;
   UINT32 Index;
   UINT32 Offset;
   CHAR8  Buffer[40];  // Used for AsciiName param of ParseAcpi
 
-  IdMappingPtr = Ptr + MappingOffset;
   Index = 0;
-  while (Index < MappingCount) {
+  Offset = 0;
+
+  while ((Index < MappingCount) &&
+         (Offset < Length)) {
     AsciiSPrint (
       Buffer,
       sizeof (Buffer),
       "ID Mapping [%d]",
       Index
       );
-    Offset = ParseAcpi (
-               TRUE,
-               4,
-               Buffer,
-               IdMappingPtr,
-               20,
-               PARSER_PARAMS (IortNodeIdMappingParser)
-               );
-    IdMappingPtr += Offset;
+    Offset += ParseAcpi (
+                TRUE,
+                4,
+                Buffer,
+                Ptr + Offset,
+                Length - Offset,
+                PARSER_PARAMS (IortNodeIdMappingParser)
+                );
     Index++;
   }
+
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < MappingCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid ID Mapping Count. IdMappingCount = %d. " \
+        L"IdMappingBufferSize = %d.\n",
+      MappingCount,
+      Length
+      );
+  }
 }
 
 /**
@@ -317,8 +322,6 @@ DumpIortNodeSmmuV1V2 (
   UINT32 Offset;
   CHAR8  Buffer[50];  // Used for AsciiName param of ParseAcpi
 
-  UINT8* ArrayPtr;
-
   ParseAcpi (
     TRUE,
     2,
@@ -328,51 +331,97 @@ DumpIortNodeSmmuV1V2 (
     PARSER_PARAMS (IortNodeSmmuV1V2Parser)
     );
 
-  ArrayPtr = Ptr + *InterruptContextOffset;
+  // 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;
-  while (Index < *InterruptContextCount) {
+
+  while ((Index < *InterruptContextCount) &&
+         (Offset < Length)) {
     AsciiSPrint (
       Buffer,
       sizeof (Buffer),
       "Context Interrupts Array [%d]",
       Index
       );
-    Offset = ParseAcpi (
-               TRUE,
-               4,
-               Buffer,
-               ArrayPtr,
-               8,
-               PARSER_PARAMS (InterruptArrayParser)
-               );
-    ArrayPtr += Offset;
+    Offset += ParseAcpi (
+                TRUE,
+                4,
+                Buffer,
+                Ptr + Offset,
+                Length - Offset,
+                PARSER_PARAMS (InterruptArrayParser)
+                );
     Index++;
   }
 
-  ArrayPtr = Ptr + *PmuInterruptOffset;
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *InterruptContextCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid Context Interrupt count. InterruptContextCount = %d. " \
+        L"SMMUv1v2BufferSize = %d.\n",
+      *InterruptContextCount,
+      Length
+      );
+    return;
+  }
+
+  Offset = *PmuInterruptOffset;
   Index = 0;
-  while (Index < *PmuInterruptCount) {
+
+  while ((Index < *PmuInterruptCount) &&
+         (Offset < Length)) {
     AsciiSPrint (
       Buffer,
       sizeof (Buffer),
       "PMU Interrupts Array [%d]",
       Index
       );
-    Offset = ParseAcpi (
-               TRUE,
-               4,
-               Buffer,
-               ArrayPtr,
-               8,
-               PARSER_PARAMS (InterruptArrayParser)
-               );
-    ArrayPtr += Offset;
+    Offset += ParseAcpi (
+                TRUE,
+                4,
+                Buffer,
+                Ptr + Offset,
+                Length - Offset,
+                PARSER_PARAMS (InterruptArrayParser)
+                );
     Index++;
   }
 
-  if (*IortIdMappingCount != 0) {
-    DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *PmuInterruptCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid PMU Interrupt count. PmuInterruptCount = %d. " \
+        L"SMMUv1v2BufferSize = %d.\n",
+      *PmuInterruptCount,
+      Length
+      );
+    return;
   }
+
+  DumpIortNodeIdMappings (
+    Ptr + MappingOffset,
+    Length - MappingOffset,
+    MappingCount
+    );
 }
 
 /**
@@ -402,9 +451,11 @@ DumpIortNodeSmmuV3 (
     PARSER_PARAMS (IortNodeSmmuV3Parser)
     );
 
-  if (*IortIdMappingCount != 0) {
-    DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
-  }
+  DumpIortNodeIdMappings (
+    Ptr + MappingOffset,
+    Length - MappingOffset,
+    MappingCount
+    );
 }
 
 /**
@@ -422,40 +473,64 @@ DumpIortNodeIts (
 {
   UINT32 Offset;
   UINT32 Index;
-  UINT8* ItsIdPtr;
   CHAR8  Buffer[80];  // Used for AsciiName param of ParseAcpi
 
   Offset = ParseAcpi (
-             TRUE,
-             2,
-             "ITS Node",
-             Ptr,
-             Length,
-             PARSER_PARAMS (IortNodeItsParser)
-             );
+            TRUE,
+            2,
+            "ITS Node",
+            Ptr,
+            Length,
+            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;
+  }
 
-  ItsIdPtr = Ptr + Offset;
   Index = 0;
-  while (Index < *ItsCount) {
+
+  while ((Index < *ItsCount) &&
+         (Offset < Length)) {
     AsciiSPrint (
       Buffer,
       sizeof (Buffer),
       "GIC ITS Identifier Array [%d]",
       Index
       );
-    Offset = ParseAcpi (
-               TRUE,
-               4,
-               Buffer,
-               ItsIdPtr,
-               4,
-               PARSER_PARAMS (ItsIdParser)
-               );
-    ItsIdPtr += Offset;
+    Offset += ParseAcpi (
+                TRUE,
+                4,
+                Buffer,
+                Ptr + Offset,
+                Length - Offset,
+                PARSER_PARAMS (ItsIdParser)
+                );
     Index++;
   }
 
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *ItsCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid GIC ITS identifier count. ItsCount = %d. " \
+        L"ItsGroupBufferSize = %d.\n",
+      *ItsCount,
+      Length
+      );
+  }
+
   // Note: ITS does not have the ID Mappings Array
+
 }
 
 /**
@@ -478,8 +553,6 @@ DumpIortNodeNamedComponent (
 {
   UINT32 Offset;
   UINT32 Index;
-  UINT8* DeviceNamePtr;
-  UINT32 DeviceNameLength;
 
   Offset = ParseAcpi (
              TRUE,
@@ -490,19 +563,35 @@ DumpIortNodeNamedComponent (
              PARSER_PARAMS (IortNodeNamedComponentParser)
              );
 
-  DeviceNamePtr = Ptr + Offset;
   // Estimate the Device Name length
-  DeviceNameLength = Length - Offset - (MappingCount * 20);
   PrintFieldName (2, L"Device Object Name");
   Index = 0;
-  while ((Index < DeviceNameLength) && (DeviceNamePtr[Index] != 0)) {
-    Print (L"%c", DeviceNamePtr[Index++]);
+
+  while ((*(Ptr + Offset) != 0) &&
+         (Offset < Length)) {
+    Print (L"%c", *(Ptr + Offset));
+    Offset++;
   }
   Print (L"\n");
 
-  if (*IortIdMappingCount != 0) {
-    DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
+  // Cross-check the string length with the size of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (*(Ptr + Offset) != '\0')) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid Device object name string. " \
+        L"NamedComponentBufferSize = %d.\n",
+      Length
+      );
+    return;
   }
+
+  DumpIortNodeIdMappings (
+    Ptr + MappingOffset,
+    Length - MappingOffset,
+    MappingCount
+    );
 }
 
 /**
@@ -532,9 +621,11 @@ DumpIortNodeRootComplex (
     PARSER_PARAMS (IortNodeRootComplexParser)
     );
 
-  if (*IortIdMappingCount != 0) {
-    DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
-  }
+  DumpIortNodeIdMappings (
+    Ptr + MappingOffset,
+    Length - MappingOffset,
+    MappingCount
+    );
 }
 
 /**
@@ -562,19 +653,13 @@ DumpIortNodePmcg (
     Ptr,
     Length,
     PARSER_PARAMS (IortNodePmcgParser)
-  );
+    );
 
-  if (*IortIdMappingCount != 0) {
-    DumpIortNodeIdMappings (Ptr, MappingCount, MappingOffset);
-  }
-
-  if (*IortIdMappingCount > 1) {
-    IncrementErrorCount ();
-    Print (
-      L"ERROR: ID mapping must not be greater than 1. Id Mapping Count =%d\n",
-      *IortIdMappingCount
-      );
-  }
+  DumpIortNodeIdMappings (
+    Ptr + MappingOffset,
+    Length - MappingOffset,
+    MappingCount
+    );
 }
 
 /**
@@ -621,23 +706,61 @@ ParseAcpiIort (
     AcpiTableLength,
     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;
 
-  while ((Index < *IortNodeCount) && (Offset < AcpiTableLength)) {
+  // Parse the specified number of IORT nodes or the IORT table buffer length.
+  // Whichever is minimum.
+  while ((Index++ < *IortNodeCount) &&
+         (Offset < AcpiTableLength)) {
     // Parse the IORT Node Header
     ParseAcpi (
       FALSE,
       0,
       "IORT Node Header",
       NodePtr,
-      16,
+      AcpiTableLength - Offset,
       PARSER_PARAMS (IortNodeHeaderParser)
       );
-    if (*IortNodeLength == 0) {
+
+    // 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: Parser error. Invalid table data.\n");
+      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 ();
+      Print (
+        L"ERROR: Invalid IORT node length. IortNodeLength = %d. " \
+          L"RemainingTableBufferLength = %d. IORT parsing aborted.\n",
+        *IortNodeLength,
+        AcpiTableLength - Offset
+        );
       return;
     }
 
@@ -689,15 +812,31 @@ ParseAcpiIort (
           *IortNodeLength,
           *IortIdMappingCount,
           *IortIdMappingOffset
-        );
+          );
         break;
 
       default:
-        IncrementErrorCount ();
-        Print (L"ERROR: Unsupported IORT Node type = %d\n", *IortNodeType);
+        if (GetConsistencyChecking ()) {
+          IncrementErrorCount ();
+          Print (L"ERROR: Unsupported IORT Node type = %d\n", *IortNodeType);
+        }
+        break;
     } // switch
 
     NodePtr += (*IortNodeLength);
     Offset += (*IortNodeLength);
   } // while
+
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *IortNodeCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid IORT node count. IortNodeCount = %d. " \
+        L"AcpiTableLength = %d.\n",
+      *IortNodeCount,
+      AcpiTableLength
+      );
+  }
 }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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. Test against buffer overruns.

3. Allow silencing ACPI table content validation errors which do not
cause table parsing to fail.

4. Remove redundant forward function declarations by repositioning
blocks of code.

5. Convert a 'do-while' loop for parsing GTDT table body into a 'while'
block for consistency with other table parsers.

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

Changes can be seen at: https://github.com/KrzysztofKoch1/edk2/commit/8c2ed18c7f1c44620eb86e1c9117cbccee8938ce

Notes:
    v1:
    - improve the logic in the GTDT parser [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c | 294 +++++++++++---------
 1 file changed, 170 insertions(+), 124 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
index 3b05ff3015d4a3af62dd9fab057c32369a456267..4e8e6f3eb50596823827d20dbb72314a583d0931 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
@@ -12,6 +12,10 @@
 #include <Library/UefiLib.h>
 #include "AcpiParser.h"
 #include "AcpiTableParser.h"
+#include "AcpiView.h"
+
+// "The number of GT Block Timers must be less than or equal to 8"
+#define GT_BLOCK_TIMER_COUNT_MAX 8
 
 // Local variables
 STATIC CONST UINT32* GtdtPlatformTimerCount;
@@ -20,7 +24,6 @@ STATIC CONST UINT8*  PlatformTimerType;
 STATIC CONST UINT16* PlatformTimerLength;
 STATIC CONST UINT32* GtBlockTimerCount;
 STATIC CONST UINT32* GtBlockTimerOffset;
-STATIC CONST UINT16* GtBlockLength;
 STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
 
 /**
@@ -36,7 +39,21 @@ EFIAPI
 ValidateGtBlockTimerCount (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  UINT32 BlockTimerCount;
+
+  BlockTimerCount = *(UINT32*)Ptr;
+
+  if (BlockTimerCount > GT_BLOCK_TIMER_COUNT_MAX) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: Timer Count = %d. Max Timer Count is %d.",
+      BlockTimerCount,
+      GT_BLOCK_TIMER_COUNT_MAX
+      );
+  }
+}
 
 /**
   This function validates the GT Frame Number.
@@ -51,7 +68,21 @@ EFIAPI
 ValidateGtFrameNumber (
   IN UINT8* Ptr,
   IN VOID*  Context
-  );
+  )
+{
+  UINT8 FrameNumber;
+
+  FrameNumber = *(UINT8*)Ptr;
+
+  if (FrameNumber >= GT_BLOCK_TIMER_COUNT_MAX) {
+    IncrementErrorCount ();
+    Print (
+      L"\nERROR: GT Frame Number = %d. GT Frame Number must be in range 0-%d.",
+      FrameNumber,
+      GT_BLOCK_TIMER_COUNT_MAX - 1
+      );
+  }
+}
 
 /**
   An ACPI_PARSER array describing the ACPI GTDT Table.
@@ -96,7 +127,7 @@ STATIC CONST ACPI_PARSER GtPlatformTimerHeaderParser[] = {
 **/
 STATIC CONST ACPI_PARSER GtBlockParser[] = {
   {L"Type", 1, 0, L"%d", NULL, NULL, NULL, NULL},
-  {L"Length", 2, 1, L"%d", NULL, (VOID**)&GtBlockLength, NULL, NULL},
+  {L"Length", 2, 1, L"%d", NULL, NULL, NULL, NULL},
   {L"Reserved", 1, 3, L"%x", NULL, NULL, NULL, NULL},
   {L"Physical address (CntCtlBase)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
   {L"Timer Count", 4, 12, L"%d", NULL, (VOID**)&GtBlockTimerCount,
@@ -134,115 +165,71 @@ STATIC CONST ACPI_PARSER SBSAGenericWatchdogParser[] = {
   {L"Watchdog Timer Flags", 4, 24, L"0x%x", NULL, NULL, NULL, NULL}
 };
 
-/**
-  This function validates the GT Block timer count.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateGtBlockTimerCount (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  UINT32 BlockTimerCount;
-
-  BlockTimerCount = *(UINT32*)Ptr;
-
-  if (BlockTimerCount > 8) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: Timer Count = %d. Max Timer Count is 8.",
-      BlockTimerCount
-      );
-  }
-}
-
-/**
-  This function validates the GT Frame Number.
-
-  @param [in] Ptr     Pointer to the start of the field data.
-  @param [in] Context Pointer to context specific information e.g. this
-                      could be a pointer to the ACPI table header.
-**/
-STATIC
-VOID
-EFIAPI
-ValidateGtFrameNumber (
-  IN UINT8* Ptr,
-  IN VOID*  Context
-  )
-{
-  UINT8 FrameNumber;
-
-  FrameNumber = *(UINT8*)Ptr;
-
-  if (FrameNumber > 7) {
-    IncrementErrorCount ();
-    Print (
-      L"\nERROR: GT Frame Number = %d. GT Frame Number must be in range 0-7.",
-      FrameNumber
-      );
-  }
-}
-
 /**
   This function parses the Platform GT Block.
 
-  @param [in] Ptr     Pointer to the start of the GT Block data.
-  @param [in] Length  Length of the GT Block structure.
+  @param [in] Ptr       Pointer to the start of the GT Block data.
+  @param [in] Length    Length of the GT Block structure.
 **/
 STATIC
 VOID
 DumpGTBlock (
   IN UINT8* Ptr,
-  IN UINT32 Length
+  IN UINT16 Length
   )
 {
   UINT32 Index;
   UINT32 Offset;
-  UINT32 GTBlockTimerLength;
 
-  Offset = ParseAcpi (
-             TRUE,
-             2,
-             "GT Block",
-             Ptr,
-             Length,
-             PARSER_PARAMS (GtBlockParser)
-             );
-  GTBlockTimerLength = (*GtBlockLength - Offset) / (*GtBlockTimerCount);
-  Length -= Offset;
+  ParseAcpi (
+    TRUE,
+    2,
+    "GT Block",
+    Ptr,
+    Length,
+    PARSER_PARAMS (GtBlockParser)
+    );
 
-  if (*GtBlockTimerCount != 0) {
-    Ptr += (*GtBlockTimerOffset);
-    Index = 0;
-    while ((Index < (*GtBlockTimerCount)) && (Length >= GTBlockTimerLength)) {
-      Offset = ParseAcpi (
-                 TRUE,
-                 2,
-                 "GT Block Timer",
-                 Ptr,
-                 GTBlockTimerLength,
-                 PARSER_PARAMS (GtBlockTimerParser)
-                 );
-      // Increment by GT Block Timer structure size
-      Ptr += Offset;
-      Length -= Offset;
-      Index++;
-    }
+  // 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;
+  }
 
-    if (Length != 0) {
-      IncrementErrorCount ();
-      Print (
-        L"ERROR:GT Block Timer length mismatch. Unparsed %d bytes.\n",
-        Length
-        );
-    }
+  Offset = *GtBlockTimerOffset;
+  Index = 0;
+
+  // Parse the specified number of GT Block Timer Structures or the GT Block
+  // Structure buffer length. Whichever is minimum.
+  while ((Index++ < *GtBlockTimerCount) &&
+         (Offset < Length)) {
+    Offset += ParseAcpi (
+                TRUE,
+                2,
+                "GT Block Timer",
+                Ptr + Offset,
+                Length - Offset,
+                PARSER_PARAMS (GtBlockTimerParser)
+                );
+  }
+
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *GtBlockTimerCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid GT Block Timer count. GtBlockTimerCount = %d. " \
+        L"GtBlockBufferSize = %d.\n",
+      *GtBlockTimerCount,
+      Length
+      );
   }
 }
 
@@ -295,6 +282,7 @@ ParseAcpiGtdt (
   )
 {
   UINT32 Index;
+  UINT32 Offset;
   UINT8* TimerPtr;
 
   if (!Trace) {
@@ -310,36 +298,94 @@ ParseAcpiGtdt (
     PARSER_PARAMS (GtdtParser)
     );
 
-  if (*GtdtPlatformTimerCount != 0) {
-    TimerPtr = Ptr + (*GtdtPlatformTimerOffset);
-    Index = 0;
-    do {
-      // Parse the Platform Timer Header
-      ParseAcpi (
-        FALSE,
-        0,
-        NULL,
-        TimerPtr,
-        4,  // GT Platform Timer structure header length.
-        PARSER_PARAMS (GtPlatformTimerHeaderParser)
+  // 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;
+
+  // Parse the specified number of Platform Timer Structures or the GTDT
+  // buffer length. Whichever is minimum.
+  while ((Index++ < *GtdtPlatformTimerCount) &&
+         (Offset < AcpiTableLength)) {
+    // Parse the Platform Timer Header to obtain Length and Type
+    ParseAcpi (
+      FALSE,
+      0,
+      NULL,
+      TimerPtr,
+      AcpiTableLength - Offset,
+      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
         );
-      switch (*PlatformTimerType) {
-        case EFI_ACPI_6_2_GTDT_GT_BLOCK:
-          DumpGTBlock (TimerPtr, *PlatformTimerLength);
-          break;
-        case EFI_ACPI_6_2_GTDT_SBSA_GENERIC_WATCHDOG:
-          DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
-          break;
-        default:
+      return;
+    }
+
+    // Make sure the Platform Timer is inside the table.
+    if ((Offset + *PlatformTimerLength) > AcpiTableLength) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Invalid Platform Timer Structure length. " \
+          L"PlatformTimerLength = %d. RemainingTableBufferLength = %d. " \
+          L"GTDT parsing aborted.\n",
+        *PlatformTimerLength,
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
+    switch (*PlatformTimerType) {
+      case EFI_ACPI_6_3_GTDT_GT_BLOCK:
+        DumpGTBlock (TimerPtr, *PlatformTimerLength);
+        break;
+      case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
+        DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
+        break;
+      default:
+        if (GetConsistencyChecking ()) {
           IncrementErrorCount ();
           Print (
-            L"ERROR: INVALID Platform Timer Type = %d\n",
+            L"ERROR: Invalid Platform Timer Type = %d\n",
             *PlatformTimerType
             );
-          break;
-      } // switch
-      TimerPtr += (*PlatformTimerLength);
-      Index++;
-    } while (Index < *GtdtPlatformTimerCount);
+        }
+        break;
+    } // switch
+
+    TimerPtr += *PlatformTimerLength;
+    Offset += *PlatformTimerLength;
+  } // while
+
+  // Cross-check the substructure count with the length of the encapsulating
+  // buffer
+  if (GetConsistencyChecking () &&
+      (Index < *GtdtPlatformTimerCount)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Invalid Platform Timer Structure count. " \
+        L"GtdtPlatformTimerCount = %d. AcpiTableLength = %d.\n",
+      *GtdtPlatformTimerCount,
+      AcpiTableLength
+      );
   }
 }
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Krzysztof Koch]

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)'

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

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]

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

[Gao, Zhichao]

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)'
> 

Re: [edk2-devel] [PATCH v1 00/11] Add security checks in the Acpiview table parsers

[Alexei Fedorov]

[-- Attachment #1: Type: text/plain, Size: 54 bytes --]

Reviewed-by: Alexei Fedorov <Alexei.Fedorov@arm.com>

[-- Attachment #2: Type: text/html, Size: 60 bytes --]