Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, July 23, 2019 4:34 AM
> To: devel@edk2.groups.io; Gao, Zhichao
> Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming;
> Sean Brogan; Michael Turner
> Subject: Re: [edk2-devel] [PATCH 2/5]
> MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
> 
> On 07/22/19 06:02, Gao, Zhichao wrote:
> > From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
> >
> > Add the instance of SecurityLockAuditLib. This instance
> > has one interface SecurityLockReportEvent to log hardware
> > and software security locks info.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> >  .../SecurityLockAuditDebugLib.c               | 53 +++++++++++++++++++
> >  .../SecurityLockAuditDebugLib.inf             | 29 ++++++++++
> >  2 files changed, 82 insertions(+)
> >  create mode 100644
> MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebu
> gLib.c
> >  create mode 100644
> MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebu
> gLib.inf
> >
> > diff --git
> a/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.c
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.c
> > new file mode 100644
> > index 0000000000..c1872bc023
> > --- /dev/null
> > +++
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.c
> > @@ -0,0 +1,53 @@
> > +/** @file
> > +  This library implements the necessary functions
> > +  to log hardware and software security locks for post-processing
> > +
> > +  Copyright (c) 2018, Microsoft Corporation
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <Base.h>
> > +#include <Library/SecurityLockAuditLib.h>
> > +#include <Library/DebugLib.h>
> > +
> > +//
> > +// Used to look up lock name from LOCK_TYPE enum
> > +//
> > +CHAR8* mLockName[] = {
> > +  "SOFTWARE_LOCK",
> > +  "HARDWARE_LOCK"
> > +};
> > +
> > +
> > +/**
> > +  Function for security Lock event logging and reporting
> > +
> > +  @param[in] Module                   GUID of calling module
> > +  @param[in] Function                 Name of calling function
> > +  @param[in] LockEventText            Debug message explaining what is
> locked
> > +  @param[in] LockType                 Enumerated lock type for differentiation
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +SecurityLockReportEvent (
> > +  IN GUID           *Module,
> > +  IN CONST CHAR8    *Function,
> > +  IN CONST CHAR8    *LockEventText,
> > +  IN LOCK_TYPE      LockType
> > +  )
> > +{
> > +  UINTN LockTypeIndex;
> > +  UINTN LockNameCount;
> > +
> > +  LockTypeIndex = (UINTN)LockType;
> > +  LockNameCount = sizeof (mLockName) / sizeof (mLockName[0]);
> > +
> > +  if (LockTypeIndex < LockNameCount) {
> > +    DEBUG ((DEBUG_ERROR, "SecurityLock::LockType: %a, Module: %g,
> Function: %a, Output: %a\n", mLockName[LockTypeIndex], Module,
> Function, LockEventText));
> > +  } else {
> > +    DEBUG ((DEBUG_ERROR, "SecurityLock::LockType: %d, Module: %g,
> Function: %a, Output: %a\n", LockType, Module, Function, LockEventText));
> > +  }
> > +}
> 
> I disagree with this implementation. Security log messages are
> important, but they are not necessarily errors. DEBUG_ERROR should be
> used for errors, preferably for such that cannot be recovered from
> (DEBUG_WARN is OK for recoverable errors).


Agree that using 'DEBUG_ERROR' is improper here.


> 
> If DEBUG_INFO is deemed too "quiet" for logging security events, then we
> should introduce a new bit value for the debug bitmask, such as
> DEBUG_SECURITY. We have a number of "special purpose" bits already:
> 
> - DEBUG_LOAD
> - DEBUG_FS
> - DEBUG_POOL
> - DEBUG_PAGE
> - DEBUG_DISPATCH
> - etc
> 
> and I think we have room for DEBUG_SECURITY.


Not sure if 'DEBUG_EVENT' can be used in this case.

I think this bit is added for event-related services in the UEFI/PI core
system. However, I do not see any usage of 'DEBUG_EVENT' or 'EFI_D_EVENT'
(at least within the edk2 repository). So I am thinking whether we can
extend this bit to match this case here.

I am also fine with adding a new bit.

Best Regards,
Hao Wu


> 
> (1) And then, the log mask in this library instance should be
> 
>   DEBUG_SECURITY | DEBUG_INFO
> 
> I believe.
> 
> Thanks
> Laszlo
> 
> 
> 
> > diff --git
> a/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.inf
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.inf
> > new file mode 100644
> > index 0000000000..b641016087
> > --- /dev/null
> > +++
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDe
> bugLib.inf
> > @@ -0,0 +1,29 @@
> > +## @file
> > +#
> > +# Library that implements logging and reporting for security locks
> > +# Using DebugLib
> > +#
> > +#
> > +# Copyright (c) 2018, Microsoft Corporation
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
> > +  BASE_NAME                      = SecurityLockAuditDebugLib
> > +  FILE_GUID                      = 459d0456-d6be-458e-9cc8-e9b21745f9aa
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = SecurityLockAuditLib
> > +
> > +[Sources.common]
> > +  SecurityLockAuditDebugLib.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  MdeModulePkg/MdeModulePkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  DebugLib
> >