[edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use
Krzysztof Koch
krzysztof.koch at arm.com
Wed Jul 17 13:38:28 UTC 2019
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Carsey, Jaben via Groups.Io
Sent: Friday, July 12, 2019 15:27
To: Krzysztof Koch <Krzysztof.Koch at arm.com>; devel at edk2.groups.io
Cc: Ni, Ray <ray.ni at intel.com>; Gao, Zhichao <zhichao.gao at intel.com>; Sami Mujawar <Sami.Mujawar at arm.com>; Matteo Carlini <Matteo.Carlini at arm.com>; nd <nd at 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 at intel.com>
> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch at arm.com]
> Sent: Thursday, July 11, 2019 11:53 PM
> To: devel at edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray
> <ray.ni at intel.com>; Gao, Zhichao <zhichao.gao at intel.com>;
> Sami.Mujawar at arm.com; Matteo.Carlini at arm.com; nd at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list