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

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

[Krzysztof Koch]

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

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

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

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

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

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

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

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

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

[Krzysztof Koch]

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

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

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

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

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

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
index e40c9ef8ee4b3285faf8c6edf3cb6236ee367397..e218e45926abced1096e75441e22108db7a3a811 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
@@ -203,6 +203,20 @@ 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");
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

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

[Gao, Zhichao]

For FadtMinorRevision and X_DsdtAddress, I don't think they are required section. Maybe we should consider check the length before check them. As I know, the OVMF's FACP table doesn't have the section after flag.

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, August 15, 2019 9:11 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 03/11] ShellPkg: acpiview: FADT: Validate global pointer
> before use
> 
> Check if global pointers have been successfully updated before they are
> used for further table parsing.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Test against NULL pointers [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> index
> e40c9ef8ee4b3285faf8c6edf3cb6236ee367397..e218e45926abced1096e75441
> e22108db7a3a811 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtPars
> +++ er.c
> @@ -203,6 +203,20 @@ 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");
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

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

[Krzysztof Koch]

Hi Zhichao,

I think you're right. I will submit a v2 patch that minimizes the amount of pointer validation required for parsing the remaining tables.

Kind regards,

Krzysztof

-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com> 
Sent: Friday, August 16, 2019 8:35
To: Krzysztof Koch <Krzysztof.Koch@arm.com>; devel@edk2.groups.io
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: RE: [PATCH v1 03/11] ShellPkg: acpiview: FADT: Validate global pointer before use

For FadtMinorRevision and X_DsdtAddress, I don't think they are required section. Maybe we should consider check the length before check them. As I know, the OVMF's FACP table doesn't have the section after flag.

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, August 15, 2019 9:11 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 03/11] ShellPkg: acpiview: FADT: Validate global 
> pointer before use
> 
> Check if global pointers have been successfully updated before they 
> are used for further table parsing.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Test against NULL pointers [Krzysztof]
> 
>  
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c
> | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> index
> e40c9ef8ee4b3285faf8c6edf3cb6236ee367397..e218e45926abced1096e75441
> e22108db7a3a811 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtPars
> +++ er.c
> @@ -203,6 +203,20 @@ 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");
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

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

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

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

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

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

Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

[Gao, Zhichao]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Thursday, August 15, 2019 9:11 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 05/11] ShellPkg: acpiview: SLIT: Validate
> System Locality count
> 
> 1. Check if the 'Number of System Localities' provided can be represented in
> the SLIT table. The table 'Length' field is a 32-bit value while the 'Number of
> System Localities' field is 64-bit long.
> 
> 2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT locality count,
> then the matrix used to represent the localities is N*N bytes long. The ACPI
> table length must be big enough to fit the matrix.
> 
> 3. Remove (now) redundant 64x64 bit multiplication.

Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao

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

Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

[Krzysztof Koch]

Hi Zhichao,

Please find my comments inline marked as [Krzysztof].

Kind regards,

Krzysztof


-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com> 
Sent: Monday, August 19, 2019 2:19
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count



> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Krzysztof Koch
> Sent: Thursday, August 15, 2019 9:11 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 05/11] ShellPkg: acpiview: SLIT: 
> Validate System Locality count
> 
> 1. Check if the 'Number of System Localities' provided can be 
> represented in the SLIT table. The table 'Length' field is a 32-bit 
> value while the 'Number of System Localities' field is 64-bit long.
> 
> 2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT 
> locality count, then the matrix used to represent the localities is 
> N*N bytes long. The ACPI table length must be big enough to fit the matrix.
> 
> 3. Remove (now) redundant 64x64 bit multiplication.

Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao

[Krzysztof] If you look closely, in this patch I have removed the need to 64x64 bit multiplication. As I explain in the commit message, the specification of the SLIT table has an error. The System Locality Count is a 64-bit value while the ACPI table length field is 32-bit long. 

Consequently, after the right checks are implemented (in this patch), it is possible to operate on 32-bit values only. I believe that now MultU64x64() is no longer needed so I reverted back to the '*' multiplication operator.

Please let me know what you think.

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

Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

[Gao, Zhichao]

Sorry for missing consider of the commit message #1 and #2.
I got your point. SlitSystemLocalityCount's high 32 bit (actually high 48 bit) data is useless.
On my opinion, this field should be 2 bytes length and the spec should be updated. Then our code can be updated to match the spec.
For now, I think the checking (*SlitSystemLocalityCount > MAX_UINT16) before it is enough.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Monday, August 19, 2019 2:28 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate
> System Locality count
> 
> 
> Hi Zhichao,
> 
> Please find my comments inline marked as [Krzysztof].
> 
> Kind regards,
> 
> Krzysztof
> 
> 
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, August 19, 2019 2:19
> To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini
> <Matteo.Carlini@arm.com>; nd <nd@arm.com>
> Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate
> System Locality count
> 
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Krzysztof Koch
> > Sent: Thursday, August 15, 2019 9:11 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 05/11] ShellPkg: acpiview: SLIT:
> > Validate System Locality count
> >
> > 1. Check if the 'Number of System Localities' provided can be
> > represented in the SLIT table. The table 'Length' field is a 32-bit
> > value while the 'Number of System Localities' field is 64-bit long.
> >
> > 2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT
> > locality count, then the matrix used to represent the localities is
> > N*N bytes long. The ACPI table length must be big enough to fit the matrix.
> >
> > 3. Remove (now) redundant 64x64 bit multiplication.
> 
> Why removing? This change is added to fixed the issue build error with IA32
> multiplication of two 64 bits data.
> The change of #3 should be removed from the patch.
> Keeping the variable size as UINT64 wouldn't affect the result.
> 
> Thanks,
> Zhichao
> 
> [Krzysztof] If you look closely, in this patch I have removed the need to 64x64
> bit multiplication. As I explain in the commit message, the specification of the
> SLIT table has an error. The System Locality Count is a 64-bit value while the
> ACPI table length field is 32-bit long.
> 
> Consequently, after the right checks are implemented (in this patch), it is
> possible to operate on 32-bit values only. I believe that now MultU64x64() is
> no longer needed so I reverted back to the '*' multiplication operator.
> 
> Please let me know what you think.
> 
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Notes:
> >     v1:
> >     - Validate the 'Number of System Localities' Field [Krzysztof]
> >     - Remove redundant 64x64 bit multiplication [Krzysztof]
> >
> >
> > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c
> > |
> > 47 +++++++++++++++++---
> >  1 file changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> > .c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> > .c
> > index
> >
> 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990
> > ecb956baf91cc3b9 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> > .c
> > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitPa
> > +++ rs
> > +++ er.c
> > @@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
> >  /**
> >    Macro to get the value of a System Locality  **/ -#define
> > SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j)
> > +#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j)
> >
> >  /**
> >    This function parses the ACPI SLIT table.
> > @@ -57,9 +57,9 @@ ParseAcpiSlit (
> >    )
> >  {
> >    UINT32 Offset;
> > -  UINT64 Count;
> > -  UINT64 Index;
> > -  UINT64 LocalityCount;
> > +  UINT32 Count;
> > +  UINT32 Index;
> > +  UINT32 LocalityCount;
> >    UINT8* LocalityPtr;
> >    CHAR16 Buffer[80];  // Used for AsciiName param of ParseAcpi
> >
> > @@ -87,8 +87,45 @@ ParseAcpiSlit (
> >      return;
> >    }
> >
> > +  /*
> > +    Despite the 'Number of System Localities' being a 64-bit field in SLIT,
> > +    the maximum number of localities that can be represented in SLIT
> > + is
> > limited
> > +    by the 'Length' field of the ACPI table.
> > +
> > +    Since the ACPI table length field is 32-bit wide. The maximum number of
> > +    localities that can be represented in SLIT can be calculated as:
> > +
> > +    MaxLocality = sqrt (MAX_UINT32 - sizeof
> >
> (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEAD
> > ER))
> > +                = 65535
> > +                = MAX_UINT16
> > +  */
> > +  if (*SlitSystemLocalityCount > MAX_UINT16) {
> > +    IncrementErrorCount ();
> > +    Print (
> > +      L"ERROR: The Number of System Localities provided can't be
> > represented " \
> > +        L"in the SLIT table. SlitSystemLocalityCount = %ld. " \
> > +        L"MaxLocalityCountAllowed = %d.\n",
> > +      *SlitSystemLocalityCount,
> > +      MAX_UINT16
> > +      );
> > +    return;
> > +  }
> > +
> > +  LocalityCount = (UINT32)*SlitSystemLocalityCount;
> > +
> > +  // Make sure system localities fit in the table buffer provided  if
> > + (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) {
> > +    IncrementErrorCount ();
> > +    Print (
> > +      L"ERROR: Invalid Number of System Localities. " \
> > +        L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n",
> > +      *SlitSystemLocalityCount,
> > +      AcpiTableLength
> > +      );
> > +    return;
> > +  }
> > +
> >    LocalityPtr = Ptr + Offset;
> > -  LocalityCount = *SlitSystemLocalityCount;
> >
> >    // We only print the Localities if the count is less than 16
> >    // If the locality count is more than 16 then refer to the
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
> >
> >
> 
> 
> 

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

[Sami Mujawar]

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 15 August 2019 02:11 PM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

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

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

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

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

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

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

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

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

[Krzysztof Koch]

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

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

Notes:
    v1:
    - Test against NULL pointers [Krzysztof]

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

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
index 869e700b9beda4886bf7bc5ae4ced3ab9a59efa3..0f730a306a94329a23fbaf54b59f1833b44616ba 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -123,6 +123,24 @@ DumpDbgDeviceInfo (
     PARSER_PARAMS (DbgDevInfoParser)
     );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((GasCount == NULL)              ||
+      (NameSpaceStringLength == NULL) ||
+      (NameSpaceStringOffset == NULL) ||
+      (OEMDataLength == NULL)         ||
+      (OEMDataOffset == NULL)         ||
+      (BaseAddrRegOffset == NULL)     ||
+      (AddrSizeOffset == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient Debug Device Information Structure length. " \
+        L"Length = %d.\n",
+      Length
+      );
+    return;
+  }
+
   // GAS
   Index = 0;
   Offset = *BaseAddrRegOffset;
@@ -224,6 +242,18 @@ ParseAcpiDbg2 (
              PARSER_PARAMS (Dbg2Parser)
              );
 
+  // Check if the values used to control the parsing logic have been
+  // successfully read.
+  if ((OffsetDbgDeviceInfo == NULL) ||
+      (NumberDbgDeviceInfo == NULL)) {
+    IncrementErrorCount ();
+    Print (
+      L"ERROR: Insufficient table length. AcpiTableLength = %d\n",
+      AcpiTableLength
+      );
+    return;
+  }
+
   Offset = *OffsetDbgDeviceInfo;
   Index = 0;
 
@@ -239,6 +269,19 @@ ParseAcpiDbg2 (
       PARSER_PARAMS (DbgDevInfoHeaderParser)
       );
 
+    // Check if the values used to control the parsing logic have been
+    // successfully read.
+    if (DbgDevInfoLen == NULL) {
+      IncrementErrorCount ();
+      Print (
+        L"ERROR: Insufficient remaining table buffer length to read the " \
+          L"Debug Device Information structure's 'Length' field. " \
+          L"RemainingTableBufferLength = %d.\n",
+        AcpiTableLength - Offset
+        );
+      return;
+    }
+
     // Make sure the Debug Device Information structure lies inside the table.
     if ((Offset + *DbgDevInfoLen) > AcpiTableLength) {
       IncrementErrorCount ();
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v1 00/11] Test against invalid pointers in acpiview

[Liming Gao]

Krzysztof:
   Can you submit BZ in https://bugzilla.tianocore.org/ for this change? 

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

Re: [edk2-devel] [PATCH v1 00/11] Test against invalid pointers in acpiview

[Krzysztof Koch]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2089

Hi Liming,

Sure, no problem.

Kind regards,

Krzysztof

-----Original Message-----
From: Gao, Liming <liming.gao@intel.com> 
Sent: Friday, August 16, 2019 5:03
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; 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 00/11] Test against invalid pointers in acpiview

Krzysztof:
   Can you submit BZ in https://bugzilla.tianocore.org/ for this change? 

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

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

[Sami Mujawar]

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 15 August 2019 02:11 PM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; zhichao.gao@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: [PATCH v1 00/11] Test against invalid pointers in acpiview

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

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

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

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

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

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

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

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

Re: [edk2-devel] [PATCH v1 00/11] Test against invalid pointers in acpiview

[Gao, Zhichao]

For 1-2, 4, 6-11: Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

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