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.100, mailfrom: zhichao.gao@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Thu, 18 Jul 2019 18:21:31 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jul 2019 18:21:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,280,1559545200"; d="scan'208";a="162258357" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga008.jf.intel.com with ESMTP; 18 Jul 2019 18:21:30 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 18 Jul 2019 18:21:30 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 18 Jul 2019 18:21:30 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.134]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.22]) with mapi id 14.03.0439.000; Fri, 19 Jul 2019 09:21:26 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "krzysztof.koch@arm.com" CC: "Carsey, Jaben" , "Ni, Ray" , "Sami.Mujawar@arm.com" , "Matteo.Carlini@arm.com" , "nd@arm.com" Subject: Re: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Thread-Topic: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validate global pointers before use Thread-Index: AQHVOH6FxKNzgSPVRUaGbX3X9FNEEKbRLkuQ Date: Fri, 19 Jul 2019 01:21:26 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B808E07@SHSMSX101.ccr.corp.intel.com> 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-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Sorry for late review. > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Krzysztof Koch > Sent: Friday, July 12, 2019 2: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: [edk2-devel] [PATCH v1 01/11] ShellPkg: acpiview: FADT: Validat= e > global pointers before use >=20 > 1. Check if the global pointer have been successfully updated before the= y are > later used to control the parsing logic in the FADT acpiview parser. >=20 > 2. Remove redundant forward function declarations by repositioning block= s > of code. >=20 > 3. Allow silencing ACPI table content validation errors which do not cau= se > 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/FadtPars > +++ er.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 ta= ble > 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)) { 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 exampl= e, 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 =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.OemTabl= eId) > { > + 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 >=20 >=20 >=20