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

["Laszlo Ersek" <lersek@redhat.com>]

(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/SecurityLockAuditDebugLib.c
>>  create mode 100644 MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.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/SecurityLockAuditDebugLib.c b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c
>> new file mode 100644
>> index 0000000000..c1872bc023
>> --- /dev/null
>> +++ b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.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/SecurityLockAuditDebugLib.inf b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
>> new file mode 100644
>> index 0000000000..b641016087
>> --- /dev/null
>> +++ b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.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
>>
>