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

Krzysztof Koch posted 11 patches 21 weeks ago

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

Posted by Krzysztof Koch 21 weeks ago
1. Check if the global pointer have been successfully updated before
they are later used to control the parsing logic in the FADT acpiview
parser.

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

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

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

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

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

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

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



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

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

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

Posted by Gao, Zhichao 20 weeks ago
Sorry for late review.

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

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

Thanks,
Zhichao

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


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

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

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

Posted by Carsey, Jaben 21 weeks ago
I think it would be easier to see/review these changes logically if the functional changes (1 and 3) were separate from the refactoring change (2).

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

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


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

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

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

Posted by Krzysztof Koch 20 weeks ago
Hi Jaben,

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

Kind regards,

Krzysztof

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

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

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

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




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

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

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