* [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol @ 2019-02-19 10:07 Jagadeesh Ujja 2019-02-19 13:24 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Jagadeesh Ujja @ 2019-02-19 10:07 UTC (permalink / raw) To: edk2-devel, liming.gao, chao.b.zhang, leif.lindholm, ard.biesheuvel 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. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jagadeesh Ujja <jagadeesh.ujja@arm.com> --- 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 <Protocol/SmmVariable.h> +#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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-19 10:07 [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol Jagadeesh Ujja @ 2019-02-19 13:24 ` Ard Biesheuvel 2019-02-20 5:53 ` Jagadeesh Ujja 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-02-19 13:24 UTC (permalink / raw) To: Jagadeesh Ujja, Achin Gupta, Kinney, Michael D Cc: edk2-devel@lists.01.org, Gao, Liming, Zhang, Chao B, Leif Lindholm Hello Jagadeesh, On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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 <jagadeesh.ujja@arm.com> > --- > 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 <Protocol/SmmVariable.h> > +#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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-19 13:24 ` Ard Biesheuvel @ 2019-02-20 5:53 ` Jagadeesh Ujja 2019-02-20 12:23 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Jagadeesh Ujja @ 2019-02-20 5:53 UTC (permalink / raw) To: Ard Biesheuvel Cc: Achin Gupta, Kinney, Michael D, edk2-devel@lists.01.org, Zhang, Chao B, Gao, Liming hi Ard, On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > Hello Jagadeesh, > > On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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 <jagadeesh.ujja@arm.com> > > --- > > 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 <Protocol/SmmVariable.h> > > +#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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-20 5:53 ` Jagadeesh Ujja @ 2019-02-20 12:23 ` Ard Biesheuvel 2019-02-21 9:04 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-02-20 12:23 UTC (permalink / raw) To: Jagadeesh Ujja, Zeng, Star, Yao, Jiewen Cc: Achin Gupta, Kinney, Michael D, edk2-devel@lists.01.org, Zhang, Chao B, Gao, Liming On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: > > hi Ard, > On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: > > > > Hello Jagadeesh, > > > > On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. > Let me rephrase this for the benefit of the MdeModulePkg maintainers, and ask them their opinion. Currently, the DXE runtime driver that produces the architectural varstore protocols that are based on communication with MM components living elsewhere, rely on the EFI protocol database for sequencing. I.e., after dispatch, they wait for certain protocols to be installed into the DXE protocol database by the SMM drivers before proceeding to install the variable arch protocols. This does not work for standalone MM, since it has no access to the DXE protocol database, nor is it needed, since it may be assumed that the MM execution context is fully configured by the time the DXE phase starts. Jagadeesh's proposal is to factor this out, and create two different .INFs to build the same DXE runtime driver in two different ways. This defeats the purpose of having an abstract MM communication protocol, so it is something I would like to avoid. On the other hand, is it not obvious how to parameterize this requirement in another way. For the moment, I could live with putting this into a library, and leave it up to the platform to ensure the combination of the library resolution with the driver that produces the MM communicate protocol is a sane one. Any thoughts? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-20 12:23 ` Ard Biesheuvel @ 2019-02-21 9:04 ` Laszlo Ersek 2019-02-21 9:11 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2019-02-21 9:04 UTC (permalink / raw) To: Ard Biesheuvel, Jagadeesh Ujja, Zeng, Star, Yao, Jiewen Cc: Kinney, Michael D, Gao, Liming, edk2-devel@lists.01.org, Zhang, Chao B On 02/20/19 13:23, Ard Biesheuvel wrote: > On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: >> >> hi Ard, >> On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> >>> Hello Jagadeesh, >>> >>> On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. >> > > Let me rephrase this for the benefit of the MdeModulePkg maintainers, > and ask them their opinion. > > Currently, the DXE runtime driver that produces the architectural > varstore protocols that are based on communication with MM components > living elsewhere, rely on the EFI protocol database for sequencing. > I.e., after dispatch, they wait for certain protocols to be installed > into the DXE protocol database by the SMM drivers before proceeding to > install the variable arch protocols. > > This does not work for standalone MM, since it has no access to the > DXE protocol database, nor is it needed, since it may be assumed that > the MM execution context is fully configured by the time the DXE phase > starts. > > Jagadeesh's proposal is to factor this out, and create two different > .INFs to build the same DXE runtime driver in two different ways. This > defeats the purpose of having an abstract MM communication protocol, > so it is something I would like to avoid. On the other hand, is it not > obvious how to parameterize this requirement in another way. > > For the moment, I could live with putting this into a library, and > leave it up to the platform to ensure the combination of the library > resolution with the driver that produces the MM communicate protocol > is a sane one. > > Any thoughts? I think I'm missing the gist of the library approach; still, would it be possible for affected platforms (i.e. those that depend on standalone MM) to procude the necessary DXE protocols (for unblocking the variable runtime driver) in a platform DXE driver? Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-21 9:04 ` Laszlo Ersek @ 2019-02-21 9:11 ` Ard Biesheuvel 2019-02-21 9:33 ` Zeng, Star 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-02-21 9:11 UTC (permalink / raw) To: Laszlo Ersek Cc: Jagadeesh Ujja, Zeng, Star, Yao, Jiewen, Kinney, Michael D, Gao, Liming, edk2-devel@lists.01.org, Zhang, Chao B On Thu, 21 Feb 2019 at 10:04, Laszlo Ersek <lersek@redhat.com> wrote: > > On 02/20/19 13:23, Ard Biesheuvel wrote: > > On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: > >> > >> hi Ard, > >> On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel > >> <ard.biesheuvel@linaro.org> wrote: > >>> > >>> Hello Jagadeesh, > >>> > >>> On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. > >> > > > > Let me rephrase this for the benefit of the MdeModulePkg maintainers, > > and ask them their opinion. > > > > Currently, the DXE runtime driver that produces the architectural > > varstore protocols that are based on communication with MM components > > living elsewhere, rely on the EFI protocol database for sequencing. > > I.e., after dispatch, they wait for certain protocols to be installed > > into the DXE protocol database by the SMM drivers before proceeding to > > install the variable arch protocols. > > > > This does not work for standalone MM, since it has no access to the > > DXE protocol database, nor is it needed, since it may be assumed that > > the MM execution context is fully configured by the time the DXE phase > > starts. > > > > Jagadeesh's proposal is to factor this out, and create two different > > .INFs to build the same DXE runtime driver in two different ways. This > > defeats the purpose of having an abstract MM communication protocol, > > so it is something I would like to avoid. On the other hand, is it not > > obvious how to parameterize this requirement in another way. > > > > For the moment, I could live with putting this into a library, and > > leave it up to the platform to ensure the combination of the library > > resolution with the driver that produces the MM communicate protocol > > is a sane one. > > > > Any thoughts? > > I think I'm missing the gist of the library approach; still, would it be > possible for affected platforms (i.e. those that depend on standalone > MM) to procude the necessary DXE protocols (for unblocking the variable > runtime driver) in a platform DXE driver? > Yes, that is the other option: we could create a library that unconditionally produces those protocols and hook it into the MM communication driver via NULL library class resolution. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-21 9:11 ` Ard Biesheuvel @ 2019-02-21 9:33 ` Zeng, Star 2019-02-21 9:45 ` Ard Biesheuvel 0 siblings, 1 reply; 9+ messages in thread From: Zeng, Star @ 2019-02-21 9:33 UTC (permalink / raw) To: Ard Biesheuvel, Laszlo Ersek Cc: edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Zhang, Chao B, Kinney, Michael D, star.zeng On 2019/2/21 17:11, Ard Biesheuvel wrote: > On Thu, 21 Feb 2019 at 10:04, Laszlo Ersek <lersek@redhat.com> wrote: >> >> On 02/20/19 13:23, Ard Biesheuvel wrote: >>> On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: >>>> >>>> hi Ard, >>>> On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org> wrote: >>>>> >>>>> Hello Jagadeesh, >>>>> >>>>> On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. >>>> >>> >>> Let me rephrase this for the benefit of the MdeModulePkg maintainers, >>> and ask them their opinion. >>> >>> Currently, the DXE runtime driver that produces the architectural >>> varstore protocols that are based on communication with MM components >>> living elsewhere, rely on the EFI protocol database for sequencing. >>> I.e., after dispatch, they wait for certain protocols to be installed >>> into the DXE protocol database by the SMM drivers before proceeding to >>> install the variable arch protocols. >>> >>> This does not work for standalone MM, since it has no access to the >>> DXE protocol database, nor is it needed, since it may be assumed that >>> the MM execution context is fully configured by the time the DXE phase >>> starts. >>> >>> Jagadeesh's proposal is to factor this out, and create two different >>> .INFs to build the same DXE runtime driver in two different ways. This >>> defeats the purpose of having an abstract MM communication protocol, >>> so it is something I would like to avoid. On the other hand, is it not >>> obvious how to parameterize this requirement in another way. >>> >>> For the moment, I could live with putting this into a library, and >>> leave it up to the platform to ensure the combination of the library >>> resolution with the driver that produces the MM communicate protocol >>> is a sane one. >>> >>> Any thoughts? >> >> I think I'm missing the gist of the library approach; still, would it be >> possible for affected platforms (i.e. those that depend on standalone >> MM) to procude the necessary DXE protocols (for unblocking the variable >> runtime driver) in a platform DXE driver? >> > > Yes, that is the other option: we could create a library that > unconditionally produces those protocols and hook it into the MM > communication driver via NULL library class resolution. > I am not familiar with standalone MM, either ARM. So may have no much valuable opinion. For this case, standalone MM could not install DXE protocols into DXE protocol database to notify the wrapper (VariableSmmRuntimeDxe), so need another way to install the DXE protocols, right? Could standalone MM assume the MM handler for variable is ready when MM communication driver runs? If yes, a NULL library instance should work (as a stub to install the DXE protocols in its constructor). :) Thanks, Star ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-21 9:33 ` Zeng, Star @ 2019-02-21 9:45 ` Ard Biesheuvel 2019-02-25 8:17 ` Jagadeesh Ujja 0 siblings, 1 reply; 9+ messages in thread From: Ard Biesheuvel @ 2019-02-21 9:45 UTC (permalink / raw) To: Zeng, Star Cc: Laszlo Ersek, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Zhang, Chao B, Kinney, Michael D On Thu, 21 Feb 2019 at 10:33, Zeng, Star <star.zeng@intel.com> wrote: > > On 2019/2/21 17:11, Ard Biesheuvel wrote: > > On Thu, 21 Feb 2019 at 10:04, Laszlo Ersek <lersek@redhat.com> wrote: > >> > >> On 02/20/19 13:23, Ard Biesheuvel wrote: > >>> On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: > >>>> > >>>> hi Ard, > >>>> On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel > >>>> <ard.biesheuvel@linaro.org> wrote: > >>>>> > >>>>> Hello Jagadeesh, > >>>>> > >>>>> On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. > >>>> > >>> > >>> Let me rephrase this for the benefit of the MdeModulePkg maintainers, > >>> and ask them their opinion. > >>> > >>> Currently, the DXE runtime driver that produces the architectural > >>> varstore protocols that are based on communication with MM components > >>> living elsewhere, rely on the EFI protocol database for sequencing. > >>> I.e., after dispatch, they wait for certain protocols to be installed > >>> into the DXE protocol database by the SMM drivers before proceeding to > >>> install the variable arch protocols. > >>> > >>> This does not work for standalone MM, since it has no access to the > >>> DXE protocol database, nor is it needed, since it may be assumed that > >>> the MM execution context is fully configured by the time the DXE phase > >>> starts. > >>> > >>> Jagadeesh's proposal is to factor this out, and create two different > >>> .INFs to build the same DXE runtime driver in two different ways. This > >>> defeats the purpose of having an abstract MM communication protocol, > >>> so it is something I would like to avoid. On the other hand, is it not > >>> obvious how to parameterize this requirement in another way. > >>> > >>> For the moment, I could live with putting this into a library, and > >>> leave it up to the platform to ensure the combination of the library > >>> resolution with the driver that produces the MM communicate protocol > >>> is a sane one. > >>> > >>> Any thoughts? > >> > >> I think I'm missing the gist of the library approach; still, would it be > >> possible for affected platforms (i.e. those that depend on standalone > >> MM) to procude the necessary DXE protocols (for unblocking the variable > >> runtime driver) in a platform DXE driver? > >> > > > > Yes, that is the other option: we could create a library that > > unconditionally produces those protocols and hook it into the MM > > communication driver via NULL library class resolution. > > > > I am not familiar with standalone MM, either ARM. So may have no much > valuable opinion. > > For this case, standalone MM could not install DXE protocols into DXE > protocol database to notify the wrapper (VariableSmmRuntimeDxe), so need > another way to install the DXE protocols, right? Yes > Could standalone MM assume the MM handler for variable is ready when MM > communication driver runs? Yes > If yes, a NULL library instance should work (as a stub to install the > DXE protocols in its constructor). :) > Yes, that was my suggestion. So Jagadeesh, could you please take this approach instead? - Create a library in StandaloneMmPkg with LIBRARY_CLASS = NULL and a constructor that installs the two protocols. - Update your platform so that the MM communication driver is included as ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf { <LibraryClasses> NULL|StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf } I don't think this will violate any ordering constraints, given that the drivers that have a dependency on these protocols also have a dependency on the MM communicate driver itself. Thanks, Ard. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol 2019-02-21 9:45 ` Ard Biesheuvel @ 2019-02-25 8:17 ` Jagadeesh Ujja 0 siblings, 0 replies; 9+ messages in thread From: Jagadeesh Ujja @ 2019-02-25 8:17 UTC (permalink / raw) To: Ard Biesheuvel Cc: Zeng, Star, edk2-devel@lists.01.org, Gao, Liming, Yao, Jiewen, Kinney, Michael D, Laszlo Ersek, Zhang, Chao B Hi Ard/Star On Thu, Feb 21, 2019 at 3:15 PM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Thu, 21 Feb 2019 at 10:33, Zeng, Star <star.zeng@intel.com> wrote: > > > > On 2019/2/21 17:11, Ard Biesheuvel wrote: > > > On Thu, 21 Feb 2019 at 10:04, Laszlo Ersek <lersek@redhat.com> wrote: > > >> > > >> On 02/20/19 13:23, Ard Biesheuvel wrote: > > >>> On Wed, 20 Feb 2019 at 06:53, Jagadeesh Ujja <jagadeesh.ujja@arm.com> wrote: > > >>>> > > >>>> hi Ard, > > >>>> On Tue, Feb 19, 2019 at 6:55 PM Ard Biesheuvel > > >>>> <ard.biesheuvel@linaro.org> wrote: > > >>>>> > > >>>>> Hello Jagadeesh, > > >>>>> > > >>>>> On Tue, 19 Feb 2019 at 11:47, Jagadeesh Ujja <jagadeesh.ujja@arm.com> 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. > > >>>> > > >>> > > >>> Let me rephrase this for the benefit of the MdeModulePkg maintainers, > > >>> and ask them their opinion. > > >>> > > >>> Currently, the DXE runtime driver that produces the architectural > > >>> varstore protocols that are based on communication with MM components > > >>> living elsewhere, rely on the EFI protocol database for sequencing. > > >>> I.e., after dispatch, they wait for certain protocols to be installed > > >>> into the DXE protocol database by the SMM drivers before proceeding to > > >>> install the variable arch protocols. > > >>> > > >>> This does not work for standalone MM, since it has no access to the > > >>> DXE protocol database, nor is it needed, since it may be assumed that > > >>> the MM execution context is fully configured by the time the DXE phase > > >>> starts. > > >>> > > >>> Jagadeesh's proposal is to factor this out, and create two different > > >>> .INFs to build the same DXE runtime driver in two different ways. This > > >>> defeats the purpose of having an abstract MM communication protocol, > > >>> so it is something I would like to avoid. On the other hand, is it not > > >>> obvious how to parameterize this requirement in another way. > > >>> > > >>> For the moment, I could live with putting this into a library, and > > >>> leave it up to the platform to ensure the combination of the library > > >>> resolution with the driver that produces the MM communicate protocol > > >>> is a sane one. > > >>> > > >>> Any thoughts? > > >> > > >> I think I'm missing the gist of the library approach; still, would it be > > >> possible for affected platforms (i.e. those that depend on standalone > > >> MM) to procude the necessary DXE protocols (for unblocking the variable > > >> runtime driver) in a platform DXE driver? > > >> > > > > > > Yes, that is the other option: we could create a library that > > > unconditionally produces those protocols and hook it into the MM > > > communication driver via NULL library class resolution. > > > > > > > I am not familiar with standalone MM, either ARM. So may have no much > > valuable opinion. > > > > For this case, standalone MM could not install DXE protocols into DXE > > protocol database to notify the wrapper (VariableSmmRuntimeDxe), so need > > another way to install the DXE protocols, right? > > Yes > > > Could standalone MM assume the MM handler for variable is ready when MM > > communication driver runs? > > Yes > > > If yes, a NULL library instance should work (as a stub to install the > > DXE protocols in its constructor). :) > > > > Yes, that was my suggestion. > > So Jagadeesh, could you please take this approach instead? > thanks for the inputs. Sorry i was on leave, will do the necessary changes and Soon will submit the patches for review. > - Create a library in StandaloneMmPkg with LIBRARY_CLASS = NULL and a > constructor that installs the two protocols. > - Update your platform so that the MM communication driver is included as > > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf { > <LibraryClasses> > NULL|StandaloneMmPkg/Library/VariableMmDependency/VariableMmDependency.inf > } > > I don't think this will violate any ordering constraints, given that > the drivers that have a dependency on these protocols also have a > dependency on the MM communicate driver itself. > > Thanks, > Ard. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-25 8:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-19 10:07 [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol Jagadeesh Ujja 2019-02-19 13:24 ` Ard Biesheuvel 2019-02-20 5:53 ` Jagadeesh Ujja 2019-02-20 12:23 ` Ard Biesheuvel 2019-02-21 9:04 ` Laszlo Ersek 2019-02-21 9:11 ` Ard Biesheuvel 2019-02-21 9:33 ` Zeng, Star 2019-02-21 9:45 ` Ard Biesheuvel 2019-02-25 8:17 ` Jagadeesh Ujja
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox