Re: [PATCH] MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch Protocol

[Jagadeesh Ujja <jagadeesh.ujja@arm.com>]

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