[edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use
Gao, Zhichao
zhichao.gao at intel.com
Fri Jul 19 01:21:26 UTC 2019
Sorry for late review.
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Friday, July 12, 2019 2: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: [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 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/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/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