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....@intel.com> > Cc: Bret Barkelew <bret.barke...@microsoft.com>; Wang, > Jian J <jian.j.w...@intel.com>; Wu, Hao A > <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, > Star <star.z...@intel.com>; Gao, Liming > <liming....@intel.com>; Sean Brogan > <sean.bro...@microsoft.com>; Michael Turner > <michael.tur...@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.barke...@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.w...@intel.com> > >> Cc: Hao A Wu <hao.a...@intel.com> > >> Cc: Ray Ni <ray...@intel.com> > >> Cc: Star Zeng <star.z...@intel.com> > >> Cc: Liming gao <liming....@intel.com> > >> Cc: Sean Brogan <sean.bro...@microsoft.com> > >> Cc: Michael Turner <michael.tur...@microsoft.com> > >> Cc: Bret Barkelew <bret.barke...@microsoft.com> > >> Signed-off-by: Zhichao Gao <zhichao....@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 > >> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44158): https://edk2.groups.io/g/devel/message/44158 Mute This Topic: https://groups.io/mt/32555407/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-