Re: [edk2-devel] [PATCH v2 5/6] SecurityPkg: Tcg2Smm: Added support for Standalone Mm

["Yao, Jiewen" <jiewen.yao@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 18281 bytes --]

Sorry for late response.
I am thinking what is the best way to address such dependency issue.


  1.  If we take similar design, we need add XxxMmDependency in any StandaloneMm module with DXE communication capability, right?

Now we have different rules:

  1.  The VariableMmDependency is in StandaloneMmPkg instead of MdeModulePkg
  2.  The Tcg2MmDependency is in SecurityPkg instead of StandaloneMmPkg.

I think we have a consistence way to add the dependency module.
I prefer to put it to the same package as the StandaloneMm module.
Can we move VariableMmDependency to MdeModulePkg ?


  1.  Also, I don't think a Library is absolutely needed.
It could be a DXE driver with gEfiMmCommunication2ProtocolGuid in dependency section, right?
E.g. a Tcg2MmDependencyDxe in SecurityPkg/Tcg/Smm, and VariableMmDependencyDxe in MdeModulePkg/Universal/Variable

Thank you
Yao Jiewen


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
Sent: Thursday, February 25, 2021 10:26 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 5/6] SecurityPkg: Tcg2Smm: Added support for Standalone Mm

Hi Jiewen,

Do you have any feedback on this patch based on my previous reply?

By the way, the reason I did not add this dependency library in StandaloneMmPkg was because it will make standalone package to depend on SecurityPkg, which does not seem adequate. Please let me know how you think. Thanks in advance.

Regards,
Kun

From: Kun Qin<mailto:kun.q@outlook.com>
Sent: Tuesday, February 23, 2021 17:40
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Zhang, Qi1<mailto:qi1.zhang@intel.com>; Kumar, Rahul1<mailto:rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 5/6] SecurityPkg: Tcg2Smm: Added support for Standalone Mm

Hi Jiewen,

This is essentially following the example of VariableStandaloneMm model here:
StandaloneMmPkg/Library: Install Variable Arch Protocol * tianocore/edk2@326598e (github.com)<https://github.com/tianocore/edk2/commit/326598e9b7591dc4117c453b270811f645d099b7#diff-a0bf18927da79063fc4535344728f85944571ac4dccb77448fe00d79a385e494>

The intended usage for this library, in the context of Standalone MM, is to link this library to the MM IPL driver (or any other drivers that has a dependency on gEfiMmCommunication2ProtocolGuid), which will make sure MM communicate is ready to use (and all MM drivers dispatched) before DXE core dispatch Tcg2Acpi driver. I could add an example like below in the commit message if you think that will help on the intended usage:
```
  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmmDxe.inf {
    <LibraryClasses>
      NULL| SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf
  }
```

Or if you have any other ideas on making sure of the loading order, please let me know as well.

Thanks,
Kun

From: Yao, Jiewen<mailto:jiewen.yao@intel.com>
Sent: Tuesday, February 23, 2021 17:26
To: Kun Qin<mailto:kun.q@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Zhang, Qi1<mailto:qi1.zhang@intel.com>; Kumar, Rahul1<mailto:rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 5/6] SecurityPkg: Tcg2Smm: Added support for Standalone Mm

I am not sure if Tcg2MmDependencyLib is the best solution.

It seems NULL lib instance. But I am not sure how it is used.

Can we have an example in SecurityPkg.dsc?



> -----Original Message-----
> From: Kun Qin <kun.q@outlook.com<mailto:kun.q@outlook.com>>
> Sent: Wednesday, February 10, 2021 9:25 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>;
> Zhang, Qi1 <qi1.zhang@intel.com<mailto:qi1.zhang@intel.com>>; Kumar, Rahul1 <rahul1.kumar@intel.com<mailto:rahul1.kumar@intel.com>>
> Subject: [PATCH v2 5/6] SecurityPkg: Tcg2Smm: Added support for Standalone
> Mm
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=3169
>
> This change added Standalone MM instance of Tcg2. The notify function for
> Standalone MM instance is left empty.
>
> A designated dependency library was created for DXE drivers to link as an
> anonymous library.
>
> Lastly, the support of CI build for Tcg2 Standalone MM module is added.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
> Cc: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> Cc: Qi Zhang <qi1.zhang@intel.com<mailto:qi1.zhang@intel.com>>
> Cc: Rahul Kumar <rahul1.kumar@intel.com<mailto:rahul1.kumar@intel.com>>
>
> Signed-off-by: Kun Qin <kun.q@outlook.com<mailto:kun.q@outlook.com>>
> ---
>
> Notes:
>     v2:
>     - Newly added.
>
>  SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.c   | 48
> ++++++++++++
>  SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.c                      | 71
> ++++++++++++++++++
>  SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf | 39
> ++++++++++
>  SecurityPkg/SecurityPkg.ci.yaml                                 |  1 +
>  SecurityPkg/SecurityPkg.dec                                     |  1 +
>  SecurityPkg/SecurityPkg.dsc                                     | 10 +++
>  SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.inf                    | 77
> ++++++++++++++++++++
>  7 files changed, 247 insertions(+)
>
> diff --git
> a/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.c
> b/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.c
> new file mode 100644
> index 000000000000..12b23813dce1
> --- /dev/null
> +++ b/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.c
> @@ -0,0 +1,48 @@
> +/** @file
> +  Runtime DXE part corresponding to StandaloneMM Tcg2 module.
> +
> +This module installs gTcg2MmSwSmiRegisteredGuid to notify readiness of
> +StandaloneMM Tcg2 module.
> +
> +Copyright (c) 2019 - 2021, Arm Ltd. All rights reserved.
> +Copyright (c) Microsoft Corporation.
> +
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <PiDxe.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +/**
> +  The constructor function installs gTcg2MmSwSmiRegisteredGuid to notify
> +  readiness of StandaloneMM Tcg2 module.
> +
> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
> +  @param  SystemTable   A pointer to the Management mode System Table.
> +
> +  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +Tcg2MmDependencyLibConstructor (
> +  IN EFI_HANDLE                           ImageHandle,
> +  IN EFI_SYSTEM_TABLE                     *SystemTable
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_HANDLE            Handle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +                  &Handle,
> +                  &gTcg2MmSwSmiRegisteredGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  NULL
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +  return EFI_SUCCESS;
> +}
> diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.c
> b/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.c
> new file mode 100644
> index 000000000000..9e0095efbc5e
> --- /dev/null
> +++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.c
> @@ -0,0 +1,71 @@
> +/** @file
> +  TCG2 Standalone MM driver that updates TPM2 items in ACPI table and
> registers
> +  SMI2 callback functions for Tcg2 physical presence, ClearMemory, and
> +  sample for dTPM StartMethod.
> +
> +  Caution: This module requires additional review when modified.
> +  This driver will have external input - variable and ACPINvs data in SMM mode.
> +  This external input must be validated carefully to avoid security issue.
> +
> +  PhysicalPresenceCallback() and MemoryClearCallback() will receive untrusted
> input and do some check.
> +
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "Tcg2Smm.h"
> +#include <Library/StandaloneMmMemLib.h>
> +
> +/**
> +  Notify the system that the SMM variable driver is ready.
> +**/
> +VOID
> +Tcg2NotifyMmReady (
> +  VOID
> +  )
> +{
> +  // Do nothing
> +}
> +
> +/**
> +  This function is an abstraction layer for implementation specific Mm buffer
> validation routine.
> +
> +  @param Buffer  The buffer start address to be checked.
> +  @param Length  The buffer length to be checked.
> +
> +  @retval TRUE  This buffer is valid per processor architecture and not overlap
> with SMRAM.
> +  @retval FALSE This buffer is not valid per processor architecture or overlap
> with SMRAM.
> +**/
> +BOOLEAN
> +IsBufferOutsideMmValid (
> +  IN EFI_PHYSICAL_ADDRESS  Buffer,
> +  IN UINT64                Length
> +  )
> +{
> +  return MmIsBufferOutsideMmValid (Buffer, Length);
> +}
> +
> +/**
> +  The driver's entry point.
> +
> +  It install callbacks for TPM physical presence and MemoryClear, and locate
> +  SMM variable to be used in the callback function.
> +
> +  @param[in] ImageHandle  The firmware allocated handle for the EFI image.
> +  @param[in] SystemTable  A pointer to the EFI System Table.
> +
> +  @retval EFI_SUCCESS     The entry point is executed successfully.
> +  @retval Others          Some error occurs when executing this entry point.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +InitializeTcgStandaloneMm (
> +  IN EFI_HANDLE                  ImageHandle,
> +  IN EFI_MM_SYSTEM_TABLE         *SystemTable
> +  )
> +{
> +  return InitializeTcgCommon ();
> +}
> diff --git
> a/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf
> b/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf
> new file mode 100644
> index 000000000000..5533ce2b6e6e
> --- /dev/null
> +++ b/SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf
> @@ -0,0 +1,39 @@
> +## @file
> +#  Runtime DXE part corresponding to StandaloneMM Tcg2 module.
> +#
> +#  This module installs gTcg2MmSwSmiRegisteredGuid to notify readiness of
> +#  StandaloneMM Tcg2 module.
> +#
> +# Copyright (c) Microsoft Corporation.
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = Tcg2MmDependencyLib
> +  FILE_GUID                      = 94C210EA-3113-4563-ADEB-76FE759C2F46
> +  MODULE_TYPE                    = DXE_DRIVER
> +  LIBRARY_CLASS                  = NULL
> +  CONSTRUCTOR                    = Tcg2MmDependencyLibConstructor
> +
> +#
> +# The following information is for reference only and not required by the build
> tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +#
> +
> +[Sources]
> +  Tcg2MmDependencyLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  SecurityPkg/SecurityPkg.dec
> +
> +[Guids]
> +  gTcg2MmSwSmiRegisteredGuid         ## PRODUCES             ## GUID # Install
> protocol
> +
> +[Depex]
> +  TRUE
> diff --git a/SecurityPkg/SecurityPkg.ci.yaml b/SecurityPkg/SecurityPkg.ci.yaml
> index 03be2e94ca97..d7b9e1f4e239 100644
> --- a/SecurityPkg/SecurityPkg.ci.yaml
> +++ b/SecurityPkg/SecurityPkg.ci.yaml
> @@ -31,6 +31,7 @@
>              "MdePkg/MdePkg.dec",
>              "MdeModulePkg/MdeModulePkg.dec",
>              "SecurityPkg/SecurityPkg.dec",
> +            "StandaloneMmPkg/StandaloneMmPkg.dec",
>              "CryptoPkg/CryptoPkg.dec"
>          ],
>          # For host based unit tests
> diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
> index 0970cae5c75e..dfbbb0365a2b 100644
> --- a/SecurityPkg/SecurityPkg.dec
> +++ b/SecurityPkg/SecurityPkg.dec
> @@ -383,6 +383,7 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmScrtmPolicy|1|UINT8|0x0001000E
>
>    ## Guid name to identify TPM instance.<BR><BR>
> +  #  NOTE: This Pcd must be FixedAtBuild if Standalone MM is used
>    #  TPM_DEVICE_INTERFACE_NONE means disable.<BR>
>    #  TPM_DEVICE_INTERFACE_TPM12 means TPM 1.2 DTPM.<BR>
>    #  TPM_DEVICE_INTERFACE_DTPM2 means TPM 2.0 DTPM.<BR>
> diff --git a/SecurityPkg/SecurityPkg.dsc b/SecurityPkg/SecurityPkg.dsc
> index 928bff72baa3..37242da93f3d 100644
> --- a/SecurityPkg/SecurityPkg.dsc
> +++ b/SecurityPkg/SecurityPkg.dsc
> @@ -166,6 +166,14 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
>
> Tcg2PhysicalPresenceLib|SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/Sm
> mTcg2PhysicalPresenceLib.inf
>    SmmIoLib|MdePkg/Library/SmmIoLib/SmmIoLib.inf
>
> +[LibraryClasses.common.MM_STANDALONE]
> +
> StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoin
> t/StandaloneMmDriverEntryPoint.inf
> +
> MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/Standalo
> neMmServicesTableLib.inf
> +
> Tcg2PhysicalPresenceLib|SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/Sta
> ndaloneMmTcg2PhysicalPresenceLib.inf
> +
> MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMe
> mLib.inf
> +
> HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLi
> b.inf
> +
> MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAlloca
> tionLib/StandaloneMmMemoryAllocationLib.inf
> +
>  [PcdsDynamicDefault.common.DEFAULT]
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid|{0xb6, 0xe5, 0x01, 0x8b,
> 0x19, 0x4f, 0xe8, 0x46, 0xab, 0x93, 0x1c, 0x53, 0x67, 0x1b, 0x90, 0xcc}
>    gEfiSecurityPkgTokenSpaceGuid.PcdTpm2InitializationPolicy|1
> @@ -183,6 +191,7 @@ [PcdsDynamicHii.common.DEFAULT]
>  [Components]
>    SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf
>
> SecurityPkg/Library/DxeImageAuthenticationStatusLib/DxeImageAuthentication
> StatusLib.inf
> +  SecurityPkg/Library/Tcg2MmDependencyLib/Tcg2MmDependencyLib.inf
>
>    #
>    # TPM
> @@ -317,6 +326,7 @@ [Components.IA32, Components.X64]
>    SecurityPkg/Tcg/MemoryOverwriteRequestControlLock/TcgMorLockSmm.inf
>    SecurityPkg/Tcg/TcgSmm/TcgSmm.inf
>    SecurityPkg/Tcg/Tcg2Smm/Tcg2Smm.inf
> +  SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.inf
>    SecurityPkg/Tcg/Tcg2Acpi/Tcg2Acpi.inf
>
> SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/SmmTcg2PhysicalPresenceLib
> .inf
>
> SecurityPkg/Library/SmmTcg2PhysicalPresenceLib/StandaloneMmTcg2PhysicalP
> resenceLib.inf
> diff --git a/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.inf
> b/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.inf
> new file mode 100644
> index 000000000000..746eda3e9fed
> --- /dev/null
> +++ b/SecurityPkg/Tcg/Tcg2Smm/Tcg2StandaloneMm.inf
> @@ -0,0 +1,77 @@
> +## @file
> +#  Provides ACPI methods for TPM 2.0 support
> +#
> +#  Spec Compliance Info:
> +#     "TCG ACPI Specification Version 1.2 Revision 8"
> +#     "Physical Presence Interface Specification Version 1.30 Revision 00.52"
> +#       along with
> +#     "Errata Version 0.4 for TCG PC Client Platform Physical Presence Interface
> Specification"
> +#     "Platform Reset Attack Mitigation Specification Version 1.00"
> +#    TPM2.0 ACPI device object
> +#     "TCG PC Client Platform Firmware Profile Specification for TPM Family 2.0
> Level 00 Revision 1.03 v51"
> +#       along with
> +#     "Errata for PC Client Specific Platform Firmware Profile Specification
> Version 1.0 Revision 1.03"
> +#
> +#  This driver implements TPM 2.0 definition block in ACPI table and
> +#  registers SMI callback functions for Tcg2 physical presence and
> +#  MemoryClear to handle the requests from ACPI method.
> +#
> +#  Caution: This module requires additional review when modified.
> +#  This driver will have external input - variable and ACPINvs data in SMM mode.
> +#  This external input must be validated carefully to avoid security issue.
> +#
> +# Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) Microsoft Corporation.<BR>
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Tcg2StandaloneMm
> +  FILE_GUID                      = D40F321F-5349-4724-B667-131670587861
> +  MODULE_TYPE                    = MM_STANDALONE
> +  PI_SPECIFICATION_VERSION       = 0x00010032
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = InitializeTcgStandaloneMm
> +
> +[Sources]
> +  Tcg2Smm.h
> +  Tcg2Smm.c
> +  Tcg2StandaloneMm.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  SecurityPkg/SecurityPkg.dec
> +  StandaloneMmPkg/StandaloneMmPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  StandaloneMmDriverEntryPoint
> +  MmServicesTableLib
> +  DebugLib
> +  Tcg2PhysicalPresenceLib
> +  PcdLib
> +  MemLib
> +
> +[Guids]
> +  ## SOMETIMES_PRODUCES ## Variable:L"MemoryOverwriteRequestControl"
> +  ## SOMETIMES_CONSUMES ## Variable:L"MemoryOverwriteRequestControl"
> +  gEfiMemoryOverwriteControlDataGuid
> +
> +  gEfiTpmDeviceInstanceTpm20DtpmGuid                            ## PRODUCES           ##
> GUID       # TPM device identifier
> +  gTpmNvsMmGuid                                                 ## CONSUMES
> +
> +[Protocols]
> +  gEfiSmmSwDispatch2ProtocolGuid                                ## CONSUMES
> +  gEfiSmmVariableProtocolGuid                                   ## CONSUMES
> +  gEfiMmReadyToLockProtocolGuid                                 ## CONSUMES
> +
> +[Pcd]
> +  gEfiSecurityPkgTokenSpaceGuid.PcdTpmInstanceGuid              ## CONSUMES
> +
> +[Depex]
> +  gEfiSmmSwDispatch2ProtocolGuid AND
> +  gEfiSmmVariableProtocolGuid
> --
> 2.30.0.windows.1







[-- Attachment #2: Type: text/html, Size: 35576 bytes --]