From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=jagadeesh.ujja@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 8D9DD21A00AE6 for ; Tue, 19 Feb 2019 21:53:28 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 216CC1682 for ; Tue, 19 Feb 2019 21:53:28 -0800 (PST) Received: from mail-it1-f174.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ED0D73FAA1 for ; Tue, 19 Feb 2019 21:53:27 -0800 (PST) Received: by mail-it1-f174.google.com with SMTP id z131so12557566itf.5 for ; Tue, 19 Feb 2019 21:53:27 -0800 (PST) X-Gm-Message-State: AHQUAuYeyuUh++VQJRnvmVyrtMk4u6oC/EpB8f5I675vnyc6YF4mfipv YwiJ2dR5KI7tNMcMseH017y7F+/BnB9eGUWUxaA= X-Google-Smtp-Source: AHgI3IbxJxaz2szrV64vYuhQx+crQcstNK4go3LvvoOqiy0ri0wmVG5z/AUP05uGDWs6ZYAED+BqAcmJ82W+cjC3XWc= X-Received: by 2002:a6b:fb09:: with SMTP id h9mr10991084iog.215.1550642007113; Tue, 19 Feb 2019 21:53:27 -0800 (PST) MIME-Version: 1.0 References: <1550570820-29379-1-git-send-email-jagadeesh.ujja@arm.com> In-Reply-To: From: Jagadeesh Ujja Date: Wed, 20 Feb 2019 11:23:16 +0530 X-Gmail-Original-Message-ID: Message-ID: To: Ard Biesheuvel Cc: Achin Gupta , "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Zhang, Chao B" , "Gao, Liming" Subject: Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2019 05:53:28 -0000 Content-Type: text/plain; charset="UTF-8" hi Ard, On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel wrote: > > Hello Jagadeesh, > > On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja wrote: > > > > In preparation for providing a standalone MM based non-secure variable > > runtime driver, factor out some portions that are specific to the > > traditional driver, mainly related to locating variable arch protocol > > and variable write arch protocol, which are not required to be located > > when using standalone MM based secure variable implementation. > > > > While i think this change is correct from a technical perspective, I > don't think this is the right approach. > these changes are mandatory, this is one of the possible solution. > It was a deliberate decision to expose the MM services in a way that > only the producer of the communication protocol is aware of the > implementation details, i.e., whether it is backed by tradtional MM or > standalone MM. > can you please provide more details on how "exposing the MM services" will help to resolve the issue here. if this helps, definitely i will use that. > By creating separate runtime DXE drivers that can work either with the > traditional MM or the standalone MM, you are defeating that, and so we > should discuss this at a more fundamental level, and also take into > account the other issue we ran into, where the communicate protocol > needs either the physical address of the comm buffer (in the > traditional MM case) or the virtual address (in the standalone MM > case). > > Both issues suggest that perhaps the 'abstract' MM communicate > protocol is not feasible in practice, in which case this patch would > probably be an appropriate course of action. > > If not, we should discuss how in general DXE runtime drivers that > DEPEX on protocols produced by SMM drivers should be implemented based > on this abstract MM model. One potential approach could be to > introduce a library that encapsulates this dependency, and leave it up > to the platform to make it depend on whichever dependencies it > defines. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jagadeesh Ujja > > --- > > MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h | 18 ++++ > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 9 +- > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf | 1 + > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.c | 42 +++++++++ > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.inf | 95 ++++++++++++++++++++ > > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMmRuntimeDxe.c | 45 ++++++++++ > > 6 files changed, 203 insertions(+), 7 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h > > index 9b294e6..c566660 100644 > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/PrivilegePolymorphic.h > > @@ -160,4 +160,22 @@ VariableHaveTcgProtocols ( > > VOID > > ); > > > > +/** > > + Check whether the protocol is installed or not. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmReady ( > > + VOID > > + ); > > + > > +/** > > + Check whether the protocol is installed or not. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmWriteReady ( > > + VOID > > + ); > > + > > #endif > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c > > index 85d655d..2976f04 100644 > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c > > @@ -47,7 +47,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #include "PrivilegePolymorphic.h" > > > > EFI_HANDLE mHandle = NULL; > > -EFI_SMM_VARIABLE_PROTOCOL *mSmmVariable = NULL; > > EFI_EVENT mVirtualAddressChangeEvent = NULL; > > EFI_SMM_COMMUNICATION_PROTOCOL *mSmmCommunication = NULL; > > UINT8 *mVariableBuffer = NULL; > > @@ -991,7 +990,7 @@ SmmVariableReady ( > > { > > EFI_STATUS Status; > > > > - Status = gBS->LocateProtocol (&gEfiSmmVariableProtocolGuid, NULL, (VOID **)&mSmmVariable); > > + Status = VariableLocateSmmReady (); > > if (EFI_ERROR (Status)) { > > return; > > } > > @@ -1068,12 +1067,8 @@ SmmVariableWriteReady ( > > ) > > { > > EFI_STATUS Status; > > - VOID *ProtocolOps; > > > > - // > > - // Check whether the protocol is installed or not. > > - // > > - Status = gBS->LocateProtocol (&gSmmVariableWriteGuid, NULL, (VOID **) &ProtocolOps); > > + Status = VariableLocateSmmWriteReady (); > > if (EFI_ERROR (Status)) { > > return; > > } > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf > > index bd73f7a..103acfa 100644 > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf > > @@ -41,6 +41,7 @@ > > # > > > > [Sources] > > + VariableTraditionalMmRuntimeDxe.c > > VariableSmmRuntimeDxe.c > > PrivilegePolymorphic.h > > Measurement.c > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.c > > new file mode 100644 > > index 0000000..0c039f1 > > --- /dev/null > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.c > > @@ -0,0 +1,42 @@ > > +/** @file > > + > > + Parts of the SMM/MM implementation that are specific to standalone MM > > + > > +Copyright (c) 2019, ARM Ltd. All rights reserved. > > + > > +This program and the accompanying materials > > +are licensed and made available under the terms and conditions of the BSD License > > +which accompanies this distribution. The full text of the license may be found at > > +http://opensource.org/licenses/bsd-license.php > > + > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#include "Variable.h" > > + > > + > > +/** > > + Check whether the protocol is installed or not. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmReady ( > > + VOID > > + ) > > +{ > > + return TRUE; > > +} > > + > > +/** > > + Check whether the protocol is installed or not. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmWriteReady ( > > + VOID > > + ) > > +{ > > + return TRUE; > > +} > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.inf > > new file mode 100644 > > index 0000000..eb4cd99 > > --- /dev/null > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMmRuntimeDxe.inf > > @@ -0,0 +1,95 @@ > > +## @file > > +# Runtime DXE part corresponding to SMM authenticated variable module. > > +# > > +# This module installs variable arch protocol and variable write arch protocol to provide > > +# variable service. This module need work together with SMM authenticated variable module. > > +# > > +# Caution: This module requires additional review when modified. > > +# This driver will have external input - variable data. > > +# This external input must be validated carefully to avoid security issues such as > > +# buffer overflow or integer overflow. > > +# The whole SMM authentication variable design relies on the integrity of flash part and SMM. > > +# which is assumed to be protected by platform. All variable code and metadata in flash/SMM Memory > > +# may not be modified without authorization. If platform fails to protect these resources, > > +# the authentication service provided in this driver will be broken, and the behavior is undefined. > > +# > > +# Copyright (c) 2019, ARM Ltd. All rights reserved. > > +# This program and the accompanying materials > > +# are licensed and made available under the terms and conditions of the BSD License > > +# which accompanies this distribution. The full text of the license may be found at > > +# http://opensource.org/licenses/bsd-license.php > > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > +# > > +## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = VariableSmmRuntimeDxe > > + MODULE_UNI_FILE = VariableSmmRuntimeDxe.uni > > + FILE_GUID = 64BC59FB-00AE-416A-952B-9DC1F7EA328C > > + MODULE_TYPE = DXE_RUNTIME_DRIVER > > + VERSION_STRING = 1.0 > > + ENTRY_POINT = VariableSmmRuntimeInitialize > > + > > +# > > +# The following information is for reference only and not required by the build tools. > > +# > > +# VALID_ARCHITECTURES = IA32 X64 > > +# > > +# VIRTUAL_ADDRESS_MAP_CALLBACK = VariableAddressChangeEvent > > +# > > + > > +[Sources] > > + VariableStandaloneMmRuntimeDxe.c > > + VariableSmmRuntimeDxe.c > > + PrivilegePolymorphic.h > > + Measurement.c > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + > > +[LibraryClasses] > > + MemoryAllocationLib > > + BaseLib > > + UefiBootServicesTableLib > > + DebugLib > > + UefiRuntimeLib > > + DxeServicesTableLib > > + UefiDriverEntryPoint > > + TpmMeasurementLib > > + > > +[Protocols] > > + gEfiVariableWriteArchProtocolGuid ## PRODUCES > > + gEfiVariableArchProtocolGuid ## PRODUCES > > + gEfiSmmCommunicationProtocolGuid ## CONSUMES > > + ## CONSUMES > > + ## NOTIFY > > + ## UNDEFINED # Used to do smm communication > > + gEfiSmmVariableProtocolGuid > > + gEdkiiVariableLockProtocolGuid ## PRODUCES > > + gEdkiiVarCheckProtocolGuid ## PRODUCES > > + > > +[Guids] > > + gEfiEventVirtualAddressChangeGuid ## CONSUMES ## Event > > + gEfiEventExitBootServicesGuid ## CONSUMES ## Event > > + ## CONSUMES ## GUID # Locate protocol > > + ## CONSUMES ## GUID # Protocol notify > > + gSmmVariableWriteGuid > > + > > + ## SOMETIMES_CONSUMES ## Variable:L"PK" > > + ## SOMETIMES_CONSUMES ## Variable:L"KEK" > > + ## SOMETIMES_CONSUMES ## Variable:L"SecureBoot" > > + gEfiGlobalVariableGuid > > + > > + ## SOMETIMES_CONSUMES ## Variable:L"db" > > + ## SOMETIMES_CONSUMES ## Variable:L"dbx" > > + ## SOMETIMES_CONSUMES ## Variable:L"dbt" > > + gEfiImageSecurityDatabaseGuid > > + > > +[Depex] > > + gEfiSmmCommunicationProtocolGuid > > + > > +[UserExtensions.TianoCore."ExtraFiles"] > > + VariableSmmRuntimeDxeExtra.uni > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMmRuntimeDxe.c > > new file mode 100644 > > index 0000000..049f849 > > --- /dev/null > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMmRuntimeDxe.c > > @@ -0,0 +1,45 @@ > > +/** @file > > + > > + Parts of the SMM/MM implementation that are specific to traditional MM > > + > > +Copyright (c) 2019, ARM Ltd. All rights reserved. > > +This program and the accompanying materials > > +are licensed and made available under the terms and conditions of the BSD License > > +which accompanies this distribution. The full text of the license may be found at > > +http://opensource.org/licenses/bsd-license.php > > + > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#include > > +#include "Variable.h" > > + > > +EFI_SMM_VARIABLE_PROTOCOL *mSmmVariable = NULL; > > + > > +/** > > + Check whether the protocol is installed or not. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmReady ( > > + VOID > > + ) > > +{ > > + return gBS->LocateProtocol (&gEfiSmmVariableProtocolGuid, NULL, (VOID **)&mSmmVariable); > > +} > > + > > +/** > > + Notify the system that the SMM variable write driver is ready. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +VariableLocateSmmWriteReady ( > > + VOID > > + ) > > +{ > > + VOID *ProtocolOps; > > + > > + return gBS->LocateProtocol (&gSmmVariableWriteGuid, NULL, (VOID **) &ProtocolOps); > > +} > > -- > > 2.7.4 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel