From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: jaben.carsey@intel.com) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by groups.io with SMTP; Fri, 12 Jul 2019 07:26:44 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jul 2019 07:26:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,482,1557212400"; d="scan'208";a="318014147" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 12 Jul 2019 07:26:43 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 12 Jul 2019 07:26:43 -0700 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.17]) by FMSMSX110.amr.corp.intel.com ([169.254.14.234]) with mapi id 14.03.0439.000; Fri, 12 Jul 2019 07:26:43 -0700 From: "Carsey, Jaben" To: Krzysztof Koch , "devel@edk2.groups.io" CC: "Ni, Ray" , "Gao, Zhichao" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Thread-Topic: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Thread-Index: AQHVOH6F7Wq5PK6+sUCh7z30GM1j2abHCkCQ Date: Fri, 12 Jul 2019 14:26:42 +0000 Message-ID: References: <20190712065243.3812-1-krzysztof.koch@arm.com> <20190712065243.3812-2-krzysztof.koch@arm.com> In-Reply-To: <20190712065243.3812-2-krzysztof.koch@arm.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzU3OWMwZTgtMmJjOC00YzgwLWExNDktY2QxOTVlMjBhM2I1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibFdnVkZFaDB0MkFrU01zUTBcL1VVcEkrZkMyU0xTZzFHTnF6Rlg0OWNVcVwvZjRvUmdjRlwvM0pXNDhiOEM0RUNvRSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Return-Path: jaben.carsey@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I think it would be easier to see/review these changes logically if the fun= ctional changes (1 and 3) were separate from the refactoring change (2). Reviewed-by: Jaben Carsey > -----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 ; Ni, Ray ; > Gao, Zhichao ; Sami.Mujawar@arm.com; > Matteo.Carlini@arm.com; nd@arm.com > Subject: [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global point= ers > before use >=20 > 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. >=20 > 2. Remove redundant forward function declarations by repositioning > blocks of code. >=20 > 3. Allow silencing ACPI table content validation errors which do not > cause table parsing to fail. >=20 > Signed-off-by: Krzysztof Koch > --- >=20 > Changes can be seen at: > https://github.com/KrzysztofKoch1/edk2/commit/49cc41430775fb93205e302 > 590a7d31f080c3952 >=20 > Notes: > v1: > - improve the logic in the parser [Krzysztof] >=20 > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Fadt/FadtParser.c > | 131 ++++++++------------ > 1 file changed, 51 insertions(+), 80 deletions(-) >=20 > 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 >=20 > - 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 >=20 > @par Reference(s): > @@ -12,6 +12,7 @@ > #include > #include "AcpiParser.h" > #include "AcpiTableParser.h" > +#include "AcpiView.h" >=20 > // 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 !=3D 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: Firmware Control must be zero for ARM platforms." > + ); > + } > +#endif > +} >=20 > /** > 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 !=3D 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: X Firmware Control must be zero for ARM platforms." > + ); > + } > +#endif > +} >=20 > /** > 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) =3D=3D 0) { > + IncrementErrorCount (); > + Print ( > + L"\nERROR: HW_REDUCED_ACPI flag must be set for ARM platforms." > + ); > + } > +#endif > +} >=20 > /** > An ACPI_PARSER array describing the ACPI FADT Table. > @@ -142,81 +173,6 @@ STATIC CONST ACPI_PARSER FadtParser[] =3D { > {L"Hypervisor VendorIdentity", 8, 268, L"%lx", NULL, NULL, NULL, NULL} > }; >=20 > -/** > - 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 !=3D 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 !=3D 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) =3D=3D 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 tab= le > fields. > @@ -248,12 +204,27 @@ ParseAcpiFadt ( > PARSER_PARAMS (FadtParser) > ); >=20 > + // Check if the values used to control the parsing logic have been > + // successfully read. > + if ((DsdtAddress =3D=3D NULL) || > + (FadtMinorRevision =3D=3D NULL) || > + (X_DsdtAddress =3D=3D NULL)) { > + IncrementErrorCount (); > + Print ( > + L"ERROR: Insufficient table length. AcpiTableLength =3D %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); >=20 > - if (*GetAcpiXsdtHeaderInfo ()->OemTableId !=3D > *AcpiHdrInfo.OemTableId) { > + if (GetConsistencyChecking () && > + (*GetAcpiXsdtHeaderInfo ()->OemTableId !=3D > *AcpiHdrInfo.OemTableId)) { > IncrementErrorCount (); > Print (L"ERROR: OEM Table Id does not match with RSDT/XSDT.\n"); > } > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >=20