> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@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.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/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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#44198): https://edk2.groups.io/g/devel/message/44198 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] -=-=-=-=-=-=-=-=-=-=-=-
