From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: Krzysztof Koch <krzysztof.koch@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Ni, Ray" <ray.ni@intel.com>,
"Gao, Zhichao" <zhichao.gao@intel.com>,
"Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>,
"Matteo.Carlini@arm.com" <Matteo.Carlini@arm.com>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use
Date: Fri, 12 Jul 2019 14:26:42 +0000 [thread overview]
Message-ID: <CB6E33457884FA40993F35157061515CBCCE476A@FMSMSX103.amr.corp.intel.com> (raw)
In-Reply-To: <20190712065243.3812-2-krzysztof.koch@arm.com>
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)'
>
next prev parent reply other threads:[~2019-07-12 14:26 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-12 6:52 [PATCH v1 00/11] Add security checks in the Acpiview table parsers Krzysztof Koch
2019-07-12 6:52 ` [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Krzysztof Koch
2019-07-12 14:26 ` Carsey, Jaben [this message]
2019-07-17 13:38 ` [edk2-devel] " Krzysztof Koch
2019-07-19 1:21 ` Gao, Zhichao
2019-07-12 6:52 ` [PATCH v1 02/11] ShellPkg: acpiview: SPCR: Remove redundant forward declaration Krzysztof Koch
2019-07-17 9:42 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 03/11] ShellPkg: acpiview: RSDP: Make printing table checksum optional Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 04/11] ShellPkg: acpiview: XSDT: Remove redundant ParseAcpi() call Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Add error-checking in the parsing logic Krzysztof Koch
2019-07-17 9:42 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 06/11] ShellPkg: acpiview: SRAT: " Krzysztof Koch
2019-07-17 9:41 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 07/11] ShellPkg: acpiview: MADT: " Krzysztof Koch
2019-07-17 9:40 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 08/11] ShellPkg: acpiview: PPTT: " Krzysztof Koch
2019-07-17 9:40 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 09/11] ShellPkg: acpiview: IORT: " Krzysztof Koch
2019-07-17 9:40 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 10/11] ShellPkg: acpiview: GTDT: " Krzysztof Koch
2019-07-17 9:39 ` [edk2-devel] " Alexei Fedorov
2019-07-12 6:52 ` [PATCH v1 11/11] ShellPkg: acpiview: DBG2: " Krzysztof Koch
2019-07-17 9:39 ` [edk2-devel] " Alexei Fedorov
2019-07-19 5:39 ` Gao, Zhichao
2019-07-17 9:42 ` [edk2-devel] [PATCH v1 00/11] Add security checks in the Acpiview table parsers Alexei Fedorov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CB6E33457884FA40993F35157061515CBCCE476A@FMSMSX103.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox