Re: [edk2-devel] 回复: [PATCH 1/1] MdeModulePkg/VarCheckPolicyLib: implement standalone MM version

["Masahisa Kojima" <masahisa.kojima@linaro.org>]

Hi Liming,

>   One minor comment, new added VarCheckPolicyLibStandaloneMm.inf is required
> to be listed in MdeModulePkg.dsc for build test.

Thank you for your comment. I will send v2 patch soon.

Thanks,
Masahisa

On Mon, 21 Dec 2020 at 10:27, gaoliming <gaoliming@byosoft.com.cn> wrote:
>
> Masahisa:
>   One minor comment, new added VarCheckPolicyLibStandaloneMm.inf is required
> to be listed in MdeModulePkg.dsc for build test.
>
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: bounce+27952+69077+4905953+8761045@groups.io
> > <bounce+27952+69077+4905953+8761045@groups.io> 代表 gaoliming
> > 发送时间: 2020年12月17日 9:15
> > 收件人: 'Masahisa Kojima' <masahisa.kojima@linaro.org>;
> > devel@edk2.groups.io; michael.d.kinney@intel.com
> > 抄送: 'Kun Qin' <kun.q@outlook.com>; 'Jian J Wang'
> <jian.j.wang@intel.com>;
> > 'Hao A Wu' <hao.a.wu@intel.com>; 'Ard Biesheuvel'
> > <ard.biesheuvel@arm.com>; 'Sami Mujawar' <sami.mujawar@arm.com>;
> > 'Jiewen Yao' <jiewen.yao@intel.com>; 'Supreeth Venkatesh'
> > <supreeth.venkatesh@arm.com>; 'Bret Barkelew'
> > <Bret.Barkelew@microsoft.com>
> > 主题: [edk2-devel] 回复: [PATCH 1/1] MdeModulePkg/VarCheckPolicyLib:
> > implement standalone MM version
> >
> > Masahisa:
> >   The patch is good. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> >
> >   Now, Mike proposes to create stable tag branch to include the critical
> bug
> > fix. I think this one is also the critical fix to be cherry-pick to the
> > stable tag branch.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > 发送时间: 2020年12月16日 22:19
> > > 收件人: devel@edk2.groups.io
> > > 抄送: Kun Qin <kun.q@outlook.com>; Masahisa Kojima
> > > <masahisa.kojima@linaro.org>; Jian J Wang <jian.j.wang@intel.com>; Hao
> > A
> > > Wu <hao.a.wu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Ard
> > > Biesheuvel <ard.biesheuvel@arm.com>; Sami Mujawar
> > > <sami.mujawar@arm.com>; Jiewen Yao <jiewen.yao@intel.com>; Supreeth
> > > Venkatesh <supreeth.venkatesh@arm.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>
> > > 主题: [PATCH 1/1] MdeModulePkg/VarCheckPolicyLib: implement
> > standalone
> > > MM version
> > >
> > > This commit adds the VarCheckPolicyLib that will be able to
> > > execute in the context of standalone MM.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > Co-authored-by: Kun Qin <kun.q@outlook.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > ---
> > >  MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > |  5 +-
> > >  MdeModulePkg/Library/VarCheckPolicyLib/{VarCheckPolicyLib.inf =>
> > > VarCheckPolicyLibStandaloneMm.inf} | 23 +++++----
> > >  MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.h
> > > | 42 ++++++++++++++++
> > >  MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > | 14 +++---
> > >
> > >
> > MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneMm
> > > .c                              | 50 ++++++++++++++++++++
> > >
> > MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibTraditional.c
> > > | 50 ++++++++++++++++++++
> > >  6 files changed, 165 insertions(+), 19 deletions(-)
> > >
> > > diff --git
> a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > index 077bcc8990ca..9af436d25f81 100644
> > > --- a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > +++ b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > @@ -13,11 +13,13 @@ [Defines]
> > >    MODULE_TYPE                    = DXE_RUNTIME_DRIVER
> > >    VERSION_STRING                 = 1.0
> > >    LIBRARY_CLASS                  = NULL|DXE_RUNTIME_DRIVER
> > > DXE_SMM_DRIVER
> > > -  CONSTRUCTOR                    = VarCheckPolicyLibConstructor
> > > +  CONSTRUCTOR                    =
> > > VarCheckPolicyLibTraditionalConstructor
> > >
> > >
> > >  [Sources]
> > >    VarCheckPolicyLib.c
> > > +  VarCheckPolicyLibTraditional.c
> > > +  VarCheckPolicyLib.h
> > >
> > >
> > >  [Packages]
> > > @@ -29,7 +31,6 @@ [LibraryClasses]
> > >    BaseLib
> > >    DebugLib
> > >    BaseMemoryLib
> > > -  DxeServicesLib
> > >    MemoryAllocationLib
> > >    VarCheckLib
> > >    VariablePolicyLib
> > > diff --git
> a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneM
> > > m.inf
> > > similarity index 51%
> > > copy from MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > copy to
> > >
> > MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneMm
> > > .inf
> > > index 077bcc8990ca..ab427f189a3d 100644
> > > --- a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > +++
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneM
> > > m.inf
> > > @@ -1,35 +1,41 @@
> > > -## @file VarCheckPolicyLib.inf
> > > +## @file VarCheckPolicyLibStandaloneMm.inf
> > >  # This is an instance of a VarCheck lib that leverages the business
> logic
> > > behind
> > >  # the VariablePolicy code to make its decisions.
> > >  #
> > > -# Copyright (c) Microsoft Corporation.
> > > +##
> > > +# Copyright (c) Microsoft Corporation. All rights reserved.
> > >  # SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > >  ##
> > >
> > >  [Defines]
> > >    INF_VERSION                    = 0x00010005
> > > -  BASE_NAME                      = VarCheckPolicyLib
> > > -  FILE_GUID                      =
> > > 9C28A48F-C884-4B1F-8B95-DEF125448023
> > > -  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
> > > +  BASE_NAME                      =
> > > VarCheckPolicyLibStandaloneMm
> > > +  FILE_GUID                      =
> > > 44B09E3D-5EDA-4673-ABCF-C8AE4560C8EC
> > > +  MODULE_TYPE                    = MM_STANDALONE
> > > +  PI_SPECIFICATION_VERSION       = 0x00010032
> > >    VERSION_STRING                 = 1.0
> > > -  LIBRARY_CLASS                  = NULL|DXE_RUNTIME_DRIVER
> > > DXE_SMM_DRIVER
> > > -  CONSTRUCTOR                    = VarCheckPolicyLibConstructor
> > > +  LIBRARY_CLASS                  = NULL|MM_STANDALONE
> > > +  CONSTRUCTOR                    =
> > > VarCheckPolicyLibStandaloneConstructor
> > >
> > >
> > >  [Sources]
> > >    VarCheckPolicyLib.c
> > > +  VarCheckPolicyLibStandaloneMm.c
> > > +  VarCheckPolicyLib.h
> > >
> > >
> > >  [Packages]
> > >    MdePkg/MdePkg.dec
> > >    MdeModulePkg/MdeModulePkg.dec
> > > +  StandaloneMmPkg/StandaloneMmPkg.dec
> > >
> > >
> > >  [LibraryClasses]
> > >    BaseLib
> > >    DebugLib
> > >    BaseMemoryLib
> > > -  DxeServicesLib
> > > +  MemLib
> > >    MemoryAllocationLib
> > >    VarCheckLib
> > >    VariablePolicyLib
> > > @@ -37,6 +43,5 @@ [LibraryClasses]
> > >    SafeIntLib
> > >    MmServicesTableLib
> > >
> > > -
> > >  [Guids]
> > >    gVarCheckPolicyLibMmiHandlerGuid        ## CONSUME ## Used to
> > > register for MM Communication events.
> > > diff --git a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.h
> > > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.h
> > > new file mode 100644
> > > index 000000000000..2226c8a19fec
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.h
> > > @@ -0,0 +1,42 @@
> > > +/** @file -- VarCheckPolicyLib.h
> > > +This internal header file defines the common interface of constructor
> for
> > > +VarCheckPolicyLib.
> > > +
> > > +Copyright (c) Microsoft Corporation. All rights reserved.
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#ifndef _VAR_CHECK_POLICY_LIB_H_
> > > +#define _VAR_CHECK_POLICY_LIB_H_
> > > +
> > > +/**
> > > +  Common constructor function of VarCheckPolicyLib to register VarCheck
> > > handler
> > > +  and SW MMI handlers.
> > > +
> > > +  @retval EFI_SUCCESS       The constructor executed correctly.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +VarCheckPolicyLibCommonConstructor (
> > > +  VOID
> > > +  );
> > > +
> > > +/**
> > > +  This function is wrapper function to validate the buffer.
> > > +
> > > +  @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/MMRAM.
> > > +  @retval FALSE This buffer is not valid per processor architecture or
> > > overlap with SMRAM/MMRAM.
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +VarCheckPolicyIsBufferOutsideValid (
> > > +  IN EFI_PHYSICAL_ADDRESS  Buffer,
> > > +  IN UINT64                Length
> > > +  );
> > > +
> > > +#endif // _VAR_CHECK_POLICY_LIB_H_
> > > diff --git a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > index 257aa9591303..14e1904e96d3 100644
> > > --- a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > +++ b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > @@ -12,7 +12,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >  #include <Library/DebugLib.h>
> > >  #include <Library/SafeIntLib.h>
> > >  #include <Library/MmServicesTableLib.h>
> > > -#include <Library/SmmMemLib.h>
> > >  #include <Library/BaseMemoryLib.h>
> > >  #include <Library/MemoryAllocationLib.h>
> > >
> > > @@ -23,6 +22,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >  #include <Guid/VarCheckPolicyMmi.h>
> > >
> > > +#include "VarCheckPolicyLib.h"
> > > +
> > >  //================================================
> > >  // As a VarCheck library, we're linked into the VariableServices
> > >  // and may not be able to call them indirectly. To get around this,
> > > @@ -102,7 +103,8 @@ VarCheckPolicyLibMmiHandler (
> > >    // Make sure that the buffer does not overlap SMM.
> > >    // This should be covered by the SmiManage infrastructure, but just
> to
> > be
> > > safe...
> > >    InternalCommBufferSize = *CommBufferSize;
> > > -  if (InternalCommBufferSize >
> > > VAR_CHECK_POLICY_MM_COMM_BUFFER_SIZE
> > > || !SmmIsBufferOutsideSmmValid((UINTN)CommBuffer,
> > > (UINT64)InternalCommBufferSize)) {
> > > +  if (InternalCommBufferSize >
> > > VAR_CHECK_POLICY_MM_COMM_BUFFER_SIZE ||
> > > +      !VarCheckPolicyIsBufferOutsideValid((UINTN)CommBuffer,
> > > (UINT64)InternalCommBufferSize)) {
> > >      DEBUG ((DEBUG_ERROR, "%a - Invalid CommBuffer supplied!
> > > 0x%016lX[0x%016lX]\n", __FUNCTION__, CommBuffer,
> > > InternalCommBufferSize));
> > >      return EFI_INVALID_PARAMETER;
> > >    }
> > > @@ -305,17 +307,13 @@ VarCheckPolicyLibMmiHandler (
> > >    Constructor function of VarCheckPolicyLib to register VarCheck
> handler
> > > and
> > >    SW MMI handlers.
> > >
> > > -  @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 constructor executed correctly.
> > >
> > >  **/
> > >  EFI_STATUS
> > >  EFIAPI
> > > -VarCheckPolicyLibConstructor (
> > > -  IN EFI_HANDLE             ImageHandle,
> > > -  IN EFI_SYSTEM_TABLE       *SystemTable
> > > +VarCheckPolicyLibCommonConstructor (
> > > +  VOID
> > >    )
> > >  {
> > >    EFI_STATUS    Status;
> > > diff --git
> > >
> > a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneM
> > > m.c
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneM
> > > m.c
> > > new file mode 100644
> > > index 000000000000..b283ced9d4e3
> > > --- /dev/null
> > > +++
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibStandaloneM
> > > m.c
> > > @@ -0,0 +1,50 @@
> > > +/** @file -- VarCheckPolicyLibStandaloneMm.c
> > > +This is an instance of a VarCheck lib constructor for Standalone MM.
> > > +
> > > +Copyright (c) Microsoft Corporation. All rights reserved.
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#include <Library/StandaloneMmMemLib.h>
> > > +
> > > +#include "VarCheckPolicyLib.h"
> > > +
> > > +/**
> > > +  Standalone MM constructor function of VarCheckPolicyLib to invoke
> > > common
> > > +  constructor routine.
> > > +
> > > +  @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 constructor executed correctly.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +VarCheckPolicyLibStandaloneConstructor (
> > > +  IN EFI_HANDLE             ImageHandle,
> > > +  IN EFI_MM_SYSTEM_TABLE    *SystemTable
> > > +  )
> > > +{
> > > +  return VarCheckPolicyLibCommonConstructor ();
> > > +}
> > > +
> > > +/**
> > > +  This function is wrapper function to validate the buffer.
> > > +
> > > +  @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 architectureand not
> > > overlap with MMRAM.
> > > +  @retval FALSE This buffer is not valid per processor architecture or
> > > overlap with MMRAM.
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +VarCheckPolicyIsBufferOutsideValid (
> > > +  IN EFI_PHYSICAL_ADDRESS  Buffer,
> > > +  IN UINT64                Length
> > > +  )
> > > +{
> > > +  return MmIsBufferOutsideMmValid (Buffer, Length);
> > > +}
> > > diff --git
> > >
> > a/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibTraditional.c
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibTraditional.c
> > > new file mode 100644
> > > index 000000000000..f404aaaa470c
> > > --- /dev/null
> > > +++
> > >
> > b/MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLibTraditional.c
> > > @@ -0,0 +1,50 @@
> > > +/** @file -- VarCheckPolicyLibTraditional.c
> > > +This is an instance of a VarCheck lib constructor for traditional SMM.
> > > +
> > > +Copyright (c) Microsoft Corporation. All rights reserved.
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +
> > > +**/
> > > +
> > > +#include <Library/SmmMemLib.h>
> > > +
> > > +#include "VarCheckPolicyLib.h"
> > > +
> > > +/**
> > > +  Traditional constructor function of VarCheckPolicyLib to invoke
> common
> > > +  constructor routine.
> > > +
> > > +  @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 constructor executed correctly.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +VarCheckPolicyLibTraditionalConstructor (
> > > +  IN EFI_HANDLE             ImageHandle,
> > > +  IN EFI_SYSTEM_TABLE       *SystemTable
> > > +  )
> > > +{
> > > +  return VarCheckPolicyLibCommonConstructor ();
> > > +}
> > > +
> > > +/**
> > > +  This function is wrapper function to validate the buffer.
> > > +
> > > +  @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
> > > +EFIAPI
> > > +VarCheckPolicyIsBufferOutsideValid (
> > > +  IN EFI_PHYSICAL_ADDRESS  Buffer,
> > > +  IN UINT64                Length
> > > +  )
> > > +{
> > > +  return SmmIsBufferOutsideSmmValid (Buffer, Length);
> > > +}
> > > --
> > > 2.17.1
> >
> >
> >
> >
> >
> >
> >
>
>
>
>
>
> 
>
>