[PATCH v1 0/6] Acpiview table parsers code style enhancements and refactoring

[PATCH v1 0/6] Acpiview table parsers code style enhancements and refactoring

[Krzysztof Koch]

This set of patches consists of a number of changes which make the code
structure consistent across the existing ACPI table parsers. These are
all refactoring changes which do not modify the existing functionality
of the acpiview UEFI shell tool.

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

Krzysztof Koch (6):
  ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
  ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call
  ShellPkg: acpiview: RSDP: Make code consistent with other parsers
  ShellPkg: acpiview: SRAT: Minor code style enhancements
  ShellPkg: acpiview: MADT: Split structure length validation
  ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26 +++++++++-------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8 +++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |  2 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 32 ++++++++++++++------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 30 +++++++++++++-----
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 11 +++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c |  3 +-
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c | 18 +++--------
 8 files changed, 82 insertions(+), 48 deletions(-)

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

[PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

[Krzysztof Koch]

Modify the signature of the DumpGasStruct() function to include the
buffer length parameter and to return the number of bytes parsed by
the function.

This way it becomes possible to prevent buffer overruns when dumping
Generic Address Structure's (GAS) fields in the acpiview table
parsers.

Update all existing DumpGasStruct() calls in acpiview to add the
length argument.

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

Notes:
    v1:
    - Modify DumpGasStruct() signature [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26 +++++++++++---------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8 ++++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |  2 +-
 3 files changed, 22 insertions(+), 14 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..2c47a3f848aa2dd512c53343ecf1c3c285173dd6 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
@@ -164,7 +164,7 @@ DumpDbgDeviceInfo (
   AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
   while (Index < (*GasCount)) {
     PrintFieldName (4, L"BaseAddressRegister");
-    DumpGasStruct (DataPtr, 4);
+    DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);
     PrintFieldName (4, L"Address Size");
     Print (L"0x%x\n", AddrSize[Index]);
     DataPtr += GAS_LENGTH;
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 2/6] ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call

[Krzysztof Koch]

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.

Modify remaining code to use the AcpiTableLength argument of the
ParseAcpiXsdt() function instead of a global static variable.

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

Notes:
    v1:
    - remove redundant ParseAcpi() call [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
index 4196168bff47d70c67f79f3fc1f4cdee302d460e..e39061f8e2612f2cce4aebf51a511b63b703662b 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):
@@ -60,22 +60,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 +74,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;
@@ -140,7 +130,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)'

[PATCH v1 3/6] ShellPkg: acpiview: RSDP: Make code consistent with other parsers

[Krzysztof Koch]

List ParseAcpi() function arguments one per line in order to make this
function call consistent with ParseAcpi() calls in other ACPI table
parsers.

Also, notify the user that XsdtAddress value of 0 results in RSDP
parsing being terminated and that the XSDT table will not be processed.

This effectively means that no more ACPI tables will be parsed because
of this RSDP table content error.

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

Notes:
    v1:
    - minor code style enhancements [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
index 586de7cbfb12f856c0c735b6e295c1cc32eb2ceb..bceda91386b5c070b81b2beac83e2a0102a9b64e 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
@@ -159,7 +159,14 @@ ParseAcpiRsdp (
     VerifyChecksum (TRUE, Ptr, AcpiTableLength);
   }
 
-  ParseAcpi (Trace, 0, "RSDP", Ptr, AcpiTableLength, PARSER_PARAMS (RsdpParser));
+  ParseAcpi (
+    Trace,
+    0,
+    "RSDP",
+    Ptr,
+    AcpiTableLength,
+    PARSER_PARAMS (RsdpParser)
+    );
 
   // This code currently supports parsing of XSDT table only
   // and does not parse the RSDT table. Platforms provide the
@@ -167,7 +174,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)'

[PATCH v1 4/6] ShellPkg: acpiview: SRAT: Minor code style enhancements

[Krzysztof Koch]

Minor changes to the SRAT parser code to conform with the EDKII coding
style and to make it consistent with other ACPI table parsers.

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

Notes:
    v1:
    - minor code style enhancements [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
index 075ff2a141a82b522e8aaedb7ad79249aaf5eaac..d0011ca65c17788c55555e2f2225380854e780fb 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c
@@ -234,6 +234,7 @@ ParseAcpiSrat (
              AcpiTableLength,
              PARSER_PARAMS (SratParser)
              );
+
   ResourcePtr = Ptr + Offset;
 
   while (Offset < AcpiTableLength) {
@@ -278,7 +279,7 @@ ParseAcpiSrat (
           ResourcePtr,
           *SratRALength,
           PARSER_PARAMS (SratGicITSAffinityParser)
-        );
+          );
         break;
 
       case EFI_ACPI_6_2_MEMORY_AFFINITY:
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 5/6] ShellPkg: acpiview: MADT: Split structure length validation

[Krzysztof Koch]

Split the Interrupt Controller Structure length validation in the
acpiview UEFI shell tool into two logical parts:
1. Ensuring MADT table parser forward progress.
2. Preventing MADT table buffer overruns.

Also, make the condition for infinite loop detection applicable to
all types of Interrupt Controller Structures (for all interrupt models
which can be represented in MADT). Check if the controller length
specified is shorter than the byte size of the first two fields
('Type' and 'Length') present in every valid Interrupt Controller
Structure.

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

Notes:
    v1:
    - split MADT structure length validation [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 30 ++++++++++++++------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
index 59c3df0cc8a080497b517baf36fc63f1e4ab866f..52b71f37a40733de2029373306658ca08c78c42d 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c
@@ -290,16 +290,30 @@ ParseAcpiMadt (
       PARSER_PARAMS (MadtInterruptControllerHeaderParser)
       );
 
-    if (((Offset + (*MadtInterruptControllerLength)) > AcpiTableLength) ||
-        (*MadtInterruptControllerLength < 4)) {
+    // Make sure forward progress is made.
+    if (*MadtInterruptControllerLength < 2) {
       IncrementErrorCount ();
       Print (
-         L"ERROR: Invalid Interrupt Controller Length,"
-          L" Type = %d, Length = %d\n",
-         *MadtInterruptControllerType,
-         *MadtInterruptControllerLength
-         );
-      break;
+        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) {
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 6/6] ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation

[Krzysztof Koch]

Move Performance Monitoring Counter Group (PMCG) node ID mapping count
validation from the core IORT acpiview parser logic to a dedicated
function. Now, the pointer to the validation function is passed 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, therefore, it is better to keep it away
from the code responsible for traversing the table.

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

Notes:
    v1:
    - refactor PMCG node mapping count validation [Krzysztof]

 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 32 ++++++++++++++------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
index 93f78e1a9786ed53f6b5529f478b72a220b4f8df..4d29ca2818804fb472bec0f632a87cd3c8a7cd48 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c
@@ -47,6 +47,28 @@ ValidateItsIdMappingCount (
   IN VOID*  Context
   );
 
+/**
+  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.
 
@@ -204,7 +226,7 @@ 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},
@@ -567,14 +589,6 @@ DumpIortNodePmcg (
   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
-      );
-  }
 }
 
 /**
--
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 0/6] Acpiview table parsers code style enhancements and refactoring

[Sami Mujawar]

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

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 18 July 2019 01:32 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 0/6] Acpiview table parsers code style enhancements and refactoring

This set of patches consists of a number of changes which make the code structure consistent across the existing ACPI table parsers. These are all refactoring changes which do not modify the existing functionality of the acpiview UEFI shell tool.

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

Krzysztof Koch (6):
  ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
  ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call
  ShellPkg: acpiview: RSDP: Make code consistent with other parsers
  ShellPkg: acpiview: SRAT: Minor code style enhancements
  ShellPkg: acpiview: MADT: Split structure length validation
  ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation

 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26 +++++++++-------
 ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8 +++--
 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c |  2 +-  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c | 32 ++++++++++++++------  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.c | 30 +++++++++++++-----  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c | 11 +++++--  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c |  3 +-  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c | 18 +++--------
 8 files changed, 82 insertions(+), 48 deletions(-)

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

Re: [edk2-devel] [PATCH v1 0/6] Acpiview table parsers code style enhancements and refactoring

[Carsey, Jaben]

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

Thanks
-Jaben


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Thursday, July 18, 2019 5:32 AM
> 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 0/6] Acpiview table parsers code style
> enhancements and refactoring
> 
> This set of patches consists of a number of changes which make the code
> structure consistent across the existing ACPI table parsers. These are
> all refactoring changes which do not modify the existing functionality
> of the acpiview UEFI shell tool.
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_code_style_e
> nhance_v1
> 
> Krzysztof Koch (6):
>   ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
>   ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call
>   ShellPkg: acpiview: RSDP: Make code consistent with other parsers
>   ShellPkg: acpiview: SRAT: Minor code style enhancements
>   ShellPkg: acpiview: MADT: Split structure length validation
>   ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26
> +++++++++-------
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8
> +++--
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> |  2 +-
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c |
> 32 ++++++++++++++------
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 30 +++++++++++++-----
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
> | 11 +++++--
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c |
> 3 +-
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
> | 18 +++--------
>  8 files changed, 82 insertions(+), 48 deletions(-)
> 
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 6/6] ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation

[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: [edk2-devel] [PATCH v1 5/6] ShellPkg: acpiview: MADT: Split structure length validation

[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: [edk2-devel] [PATCH v1 4/6] ShellPkg: acpiview: SRAT: Minor code style enhancements

[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: [edk2-devel] [PATCH v1 3/6] ShellPkg: acpiview: RSDP: Make code consistent with other 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 --]

Re: [edk2-devel] [PATCH v1 2/6] 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 --]

Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

[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: [edk2-devel] [PATCH v1 0/6] Acpiview table parsers code style enhancements and refactoring

[Gao, Zhichao]

One comment on 1/6. Others are good for me.
For 2-6,
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, July 18, 2019 8:32 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 0/6] Acpiview table parsers code style
> enhancements and refactoring
> 
> This set of patches consists of a number of changes which make the code
> structure consistent across the existing ACPI table parsers. These are all
> refactoring changes which do not modify the existing functionality of the
> acpiview UEFI shell tool.
> 
> Changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/612_acpiview_code_style_e
> nhance_v1
> 
> Krzysztof Koch (6):
>   ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
>   ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call
>   ShellPkg: acpiview: RSDP: Make code consistent with other parsers
>   ShellPkg: acpiview: SRAT: Minor code style enhancements
>   ShellPkg: acpiview: MADT: Split structure length validation
>   ShellPkg: acpiview: IORT: Refactor PMCG node mapping count validation
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26
> +++++++++-------
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8
> +++--
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> |  2 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Iort/IortParser.c |
> 32 ++++++++++++++------
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Madt/MadtParser.
> c | 30 +++++++++++++-----
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Rsdp/RsdpParser.c
> | 11 +++++--
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Srat/SratParser.c |
> 3 +-
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Xsdt/XsdtParser.c
> | 18 +++--------
>  8 files changed, 82 insertions(+), 48 deletions(-)
> 
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
> 

Re: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

[Gao, Zhichao]

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, July 18, 2019 8:32 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 1/6] ShellPkg: acpiview: Allow passing buffer length to
> DumpGasStruct()
> 
> Modify the signature of the DumpGasStruct() function to include the buffer
> length parameter and to return the number of bytes parsed by the function.
> 
> This way it becomes possible to prevent buffer overruns when dumping
> Generic Address Structure's (GAS) fields in the acpiview table parsers.
> 
> Update all existing DumpGasStruct() calls in acpiview to add the length
> argument.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Modify DumpGasStruct() signature [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26
> +++++++++++---------
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8
> ++++--
> 
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> |  2 +-
>  3 files changed, 22 insertions(+), 14 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..2c47a3f848aa2dd512c53343e
> cf1c3c285173dd6 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> r.c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> +++ er.c
> @@ -164,7 +164,7 @@ DumpDbgDeviceInfo (
>    AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
>    while (Index < (*GasCount)) {
>      PrintFieldName (4, L"BaseAddressRegister");
> -    DumpGasStruct (DataPtr, 4);
> +    DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);

This input length should be GAS_LENGTH. *DbgDevInfoLen is the length of the whole structure and the DataPtr is increased during the loop. Inputing such a length would give the ParseAcpi function a chance to overrun the DataPtr.

Thanks,
Zhichao

>      PrintFieldName (4, L"Address Size");
>      Print (L"0x%x\n", AddrSize[Index]);
>      DataPtr += GAS_LENGTH;
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

[Krzysztof Koch]

Hi Zhichao,

Sorry about private emails, I just mistakenly hit the wrong button. I forgot that the emails do not get sent to the mailing list unless I hit 'Reply all' (silly mistake).

I see your point. I will submit v2 of this patchset with:
> DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);
Replaced with
> DumpGasStruct (DataPtr, 4, GAS_LENGTH);

Is that ok?

Kind regards,

Krzysztof


-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com> 
Sent: Friday, July 19, 2019 9:09
To: Krzysztof Koch <Krzysztof.Koch@arm.com>
Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

HI Krzysztof,

Why do you send the email to me only? I think it is fine to send to the community. It would let more people who are interest with this component join the discuss. Maybe not . :(

Then for your comments:
Sorry,  I missed the commit message before. But I don't think keeping an inappropriate code with a commit massage to explain that is OK. If you have a plan to fix that, it should combine into one patch.
Back to the code, I don't find out the possibility of overrun of buffer when use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow.
    DevInfoPtr                         DataPtr                                   EndDevInfoPtr
---|---------------------------|------------------------------|----
     |<-- DbgDevInfoLen                                                     -> |

Thanks,
Zhichao

> -----Original Message-----
> From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com]
> Sent: Friday, July 19, 2019 3:39 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>
> Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer 
> length to
> DumpGasStruct()
> 
> Hi Zhichao,
> 
> Please see my comments inline marked as [Krzysztof]
> 
> Kind regards,
> 
> Krzysztof
> 
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Friday, July 19, 2019 2:15
> 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 1/6] ShellPkg: acpiview: Allow passing buffer 
> length to
> DumpGasStruct()
> 
> 
> 
> > -----Original Message-----
> > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> > Sent: Thursday, July 18, 2019 8:32 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 1/6] ShellPkg: acpiview: Allow passing buffer 
> > length to
> > DumpGasStruct()
> >
> > Modify the signature of the DumpGasStruct() function to include the 
> > buffer length parameter and to return the number of bytes parsed by 
> > the
> function.
> >
> > This way it becomes possible to prevent buffer overruns when dumping 
> > Generic Address Structure's (GAS) fields in the acpiview table parsers.
> >
> > Update all existing DumpGasStruct() calls in acpiview to add the 
> > length argument.
> >
> > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > ---
> >
> > Notes:
> >     v1:
> >     - Modify DumpGasStruct() signature [Krzysztof]
> >
> >  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26
> > +++++++++++---------
> >  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8
> > ++++--
> >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> > |  2 +-
> >  3 files changed, 22 insertions(+), 14 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..2c47a3f848aa2dd512c53343e
> > cf1c3c285173dd6 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> > r.c
> > +++
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> > +++ er.c
> > @@ -164,7 +164,7 @@ DumpDbgDeviceInfo (
> >    AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
> >    while (Index < (*GasCount)) {
> >      PrintFieldName (4, L"BaseAddressRegister");
> > -    DumpGasStruct (DataPtr, 4);
> > +    DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);
> 
> This input length should be GAS_LENGTH. *DbgDevInfoLen is the length 
> of the whole structure and the DataPtr is increased during the loop. 
> Inputing such a length would give the ParseAcpi function a chance to 
> overrun the DataPtr.
> 
> Thanks,
> Zhichao
> 
> [Krzysztof] You're right. This patch does not eliminate the 
> possibility for a buffer overrun when parsing the GAS structure.  This 
> patch series does not implement that yet, it's merely a refactoring 
> change in preparation for the patches which actually deal with the 
> problem. The patches which prevent buffer overruns in acpiview parsers 
> are pending on the patches which are currently in review.
> 
> In the commit message I state that the change of the DumpGasStruct() 
> function signature makes it "possible to prevent buffer overruns when 
> dumping Generic Address Structure's (GAS) fields in the acpiview table 
> parsers." The validation is coming soon.
> 
> >      PrintFieldName (4, L"Address Size");
> >      Print (L"0x%x\n", AddrSize[Index]);
> >      DataPtr += GAS_LENGTH;
> > --
> > '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: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()

[Gao, Zhichao]

> -----Original Message-----
> From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com]
> Sent: Friday, July 19, 2019 6:41 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: nd <nd@arm.com>
> Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to
> DumpGasStruct()
> 
> Hi Zhichao,
> 
> Sorry about private emails, I just mistakenly hit the wrong button. I forgot
> that the emails do not get sent to the mailing list unless I hit 'Reply all' (silly
> mistake).
> 
> I see your point. I will submit v2 of this patchset with:
> > DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);
> Replaced with
> > DumpGasStruct (DataPtr, 4, GAS_LENGTH);
> 
> Is that ok?

Yes. Thanks for taking my advice.

Thanks,
Zhichao

> 
> Kind regards,
> 
> Krzysztof
> 
> 
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Friday, July 19, 2019 9:09
> To: Krzysztof Koch <Krzysztof.Koch@arm.com>
> Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to
> DumpGasStruct()
> 
> HI Krzysztof,
> 
> Why do you send the email to me only? I think it is fine to send to the
> community. It would let more people who are interest with this component
> join the discuss. Maybe not . :(
> 
> Then for your comments:
> Sorry,  I missed the commit message before. But I don't think keeping an
> inappropriate code with a commit massage to explain that is OK. If you have a
> plan to fix that, it should combine into one patch.
> Back to the code, I don't find out the possibility of overrun of buffer when
> use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow.
>     DevInfoPtr                         DataPtr                                   EndDevInfoPtr
> ---|---------------------------|------------------------------|----
>      |<-- DbgDevInfoLen                                                     -> |
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Krzysztof Koch [mailto:Krzysztof.Koch@arm.com]
> > Sent: Friday, July 19, 2019 3:39 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>
> > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer
> > length to
> > DumpGasStruct()
> >
> > Hi Zhichao,
> >
> > Please see my comments inline marked as [Krzysztof]
> >
> > Kind regards,
> >
> > Krzysztof
> >
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Friday, July 19, 2019 2:15
> > 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 1/6] ShellPkg: acpiview: Allow passing buffer
> > length to
> > DumpGasStruct()
> >
> >
> >
> > > -----Original Message-----
> > > From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> > > Sent: Thursday, July 18, 2019 8:32 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 1/6] ShellPkg: acpiview: Allow passing buffer
> > > length to
> > > DumpGasStruct()
> > >
> > > Modify the signature of the DumpGasStruct() function to include the
> > > buffer length parameter and to return the number of bytes parsed by
> > > the
> > function.
> > >
> > > This way it becomes possible to prevent buffer overruns when dumping
> > > Generic Address Structure's (GAS) fields in the acpiview table parsers.
> > >
> > > Update all existing DumpGasStruct() calls in acpiview to add the
> > > length argument.
> > >
> > > Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> > > ---
> > >
> > > Notes:
> > >     v1:
> > >     - Modify DumpGasStruct() signature [Krzysztof]
> > >
> > >  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c              | 26
> > > +++++++++++---------
> > >  ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h              |  8
> > > ++++--
> > >
> > >
> >
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c
> > > |  2 +-
> > >  3 files changed, 22 insertions(+), 14 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..2c47a3f848aa2dd512c53343e
> > > cf1c3c285173dd6 100644
> > > ---
> > >
> >
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse
> > > r.c
> > > +++
> > >
> >
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars
> > > +++ er.c
> > > @@ -164,7 +164,7 @@ DumpDbgDeviceInfo (
> > >    AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset));
> > >    while (Index < (*GasCount)) {
> > >      PrintFieldName (4, L"BaseAddressRegister");
> > > -    DumpGasStruct (DataPtr, 4);
> > > +    DumpGasStruct (DataPtr, 4, *DbgDevInfoLen);
> >
> > This input length should be GAS_LENGTH. *DbgDevInfoLen is the length
> > of the whole structure and the DataPtr is increased during the loop.
> > Inputing such a length would give the ParseAcpi function a chance to
> > overrun the DataPtr.
> >
> > Thanks,
> > Zhichao
> >
> > [Krzysztof] You're right. This patch does not eliminate the
> > possibility for a buffer overrun when parsing the GAS structure.  This
> > patch series does not implement that yet, it's merely a refactoring
> > change in preparation for the patches which actually deal with the
> > problem. The patches which prevent buffer overruns in acpiview parsers
> > are pending on the patches which are currently in review.
> >
> > In the commit message I state that the change of the DumpGasStruct()
> > function signature makes it "possible to prevent buffer overruns when
> > dumping Generic Address Structure's (GAS) fields in the acpiview table
> > parsers." The validation is coming soon.
> >
> > >      PrintFieldName (4, L"Address Size");
> > >      Print (L"0x%x\n", AddrSize[Index]);
> > >      DataPtr += GAS_LENGTH;
> > > --
> > > '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.