From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::142; helo=mail-it1-x142.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x142.google.com (mail-it1-x142.google.com [IPv6:2607:f8b0:4864:20::142]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B27D32194EB7D for ; Tue, 19 Feb 2019 05:25:09 -0800 (PST) Received: by mail-it1-x142.google.com with SMTP id m137so5968281ita.0 for ; Tue, 19 Feb 2019 05:25:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=h3j/5vkM6dQTK4FC2EZjG8Xq8Ssxi9FTinhNyBeMLwE=; b=imkkpFnmBy3CJwUT53RhYNx5jQHP28k21FG4sAlgMsWp31zmX2z63bGhpu/+4HFHb1 OBHdIDraoC922mvAIMLAl6d8aFsy0Ti6CFDVhur2EO78Mq/E8TjlNHkjo0/ClV67fLBs zliPcmiK2iQdAVEV4wEXJEaxbHGttllHD0yH2cJceh86oKJj3es3rN3UBmGOODpazrne ZXQ3+7hWiowzPTXt8Aruco6HmKkEsLY2W8B1EV01/Sg5HUEw3Pi0gBj6Nm4IKypGTPzT aDWq3aEnk9nXCmblLP/XDC/+oW6EM3as40s7yhjBHJooA/Hwe7te5SxjN9jLsNGydX2o EE6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=h3j/5vkM6dQTK4FC2EZjG8Xq8Ssxi9FTinhNyBeMLwE=; b=Q+aVrsBUUzIZoW0jdb/ni4xj9qfuTyC7uoZsMH1/1ZxrxlI3ca65GTe73I8ewijIer OVrbdECLnzYR4XAdxDnpp5TrldhZgDRjuPN5HSQbmlqN3tmnk3NahGDObSXswm9wt2rc Bvems7pEVmZDWdRYdfSfHBtumLl+M8e8lkhw6oDrwvc+fphdfZfhTaAv9B6UNaC1foEK xKqwe+cqmNV9gmKVhKaXUPCcAvqP7cnOipLGa3kMILUH5EfecckEiWdGJfN59dHo6H8f 8uvmHfsHDECPg84IaZ8RQdfdBaWbAJINQLvVozfeleCkzecj9dLIGjqF3bi+IRY9ddFP wAIg== X-Gm-Message-State: AHQUAuYajtMAIhJFVV2uMYXZXEXlKtZrea33aX307I9o+pkmyiJm0Nxl c/mA8DByRaCy1CBoqcf3viub9e/i2jCRkHMWKK/H+Q== X-Google-Smtp-Source: AHgI3IZiQKUlqsmlXeDHjvF6jrLjku0VXcngo/91qfjl5vmP3Ckh/Kp3DDiDg8z5HU4F4RrTxJjgqMw4aP1K7FOqgRY= X-Received: by 2002:a6b:6511:: with SMTP id z17mr16655651iob.173.1550582707735; Tue, 19 Feb 2019 05:25:07 -0800 (PST) MIME-Version: 1.0 References: <1550570820-29379-1-git-send-email-jagadeesh.ujja@arm.com> In-Reply-To: <1550570820-29379-1-git-send-email-jagadeesh.ujja@arm.com> From: Ard Biesheuvel Date: Tue, 19 Feb 2019 14:24:56 +0100 Message-ID: To: Jagadeesh Ujja , Achin Gupta , "Kinney, Michael D" Cc: "edk2-devel@lists.01.org" , "Gao, Liming" , "Zhang, Chao B" , Leif Lindholm 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: Tue, 19 Feb 2019 13:25:10 -0000 Content-Type: text/plain; charset="UTF-8" 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. 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. 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 >