[edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
Michael D Kinney
michael.d.kinney at intel.com
Mon Jul 22 21:06:46 UTC 2019
Laszlo,
I agree the lib instance should use the lib class name.
This one should be SecurityLockAuditLibDebug.
Mike
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, July 22, 2019 1:45 PM
> To: devel at edk2.groups.io; Gao, Zhichao
> <zhichao.gao at intel.com>
> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>; Wang,
> Jian J <jian.j.wang at intel.com>; Wu, Hao A
> <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Zeng,
> Star <star.zeng at intel.com>; Gao, Liming
> <liming.gao at intel.com>; Sean Brogan
> <sean.brogan at microsoft.com>; Michael Turner
> <Michael.Turner at 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 at 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 at intel.com>
> >> Cc: Hao A Wu <hao.a.wu at intel.com>
> >> Cc: Ray Ni <ray.ni at intel.com>
> >> Cc: Star Zeng <star.zeng at intel.com>
> >> Cc: Liming gao <liming.gao at intel.com>
> >> Cc: Sean Brogan <sean.brogan at microsoft.com>
> >> Cc: Michael Turner <Michael.Turner at microsoft.com>
> >> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> >> Signed-off-by: Zhichao Gao <zhichao.gao at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list