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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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