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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Laszlo,

I agree the lib instance should use the lib class name.
This one should be SecurityLockAuditLibDebug.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, July 22, 2019 1:45 PM
> To: devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH 2/5]
> MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
> 
> (apologies for the separate message, for this patch:)
> 
> On 07/22/19 22:34, Laszlo Ersek wrote:
> > 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/SecurityLo
> ckAuditDebug
> >> Lib.c  create mode 100644
> >>
> MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLo
> ckAuditDebug
> >> Lib.inf
> 
> The edk2 nomenclature usually puts the "implementation
> hints" of a library instance after the "Lib" word. For
> example, in "SecurityLockAuditLibNull", the word "Null"
> comes after "Lib" -- and that's correct. (From patch#3.)
> 
> (2) In order to stick with this naming, I'd suggest
> renaming this lib instance to
> "SecurityLockAuditLibDebug". (Note that commit messages
> and subject lines should be updated too, not just code,
> and file names.)
> 
> If the MdeModulePkg maintainers think the current name is
> OK, I won't insist on the rename.
> 
> Thanks!
> Laszlo
> 
> >>
> >> diff --git
> >>
> a/MdeModulePkg/Library/SecurityLockAuditDebugLib/Security
> LockAuditDeb
> >> ugLib.c
> >>
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/Security
> LockAuditDeb
> >> ugLib.c
> >> new file mode 100644
> >> index 0000000000..c1872bc023
> >> --- /dev/null
> >> +++
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/Security
> LockAudi
> >> +++ tDebugLib.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).
> >
> > 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.
> >
> > (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/Security
> LockAuditDeb
> >> ugLib.inf
> >>
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/Security
> LockAuditDeb
> >> ugLib.inf
> >> new file mode 100644
> >> index 0000000000..b641016087
> >> --- /dev/null
> >> +++
> b/MdeModulePkg/Library/SecurityLockAuditDebugLib/Security
> LockAudi
> >> +++ tDebugLib.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
> >>
> >
> 
> 
>