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

["Krzysztof Koch" <krzysztof.koch@arm.com>]

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.