Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
> -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
> >
> > 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
> > Cc: Hao A Wu
> > Cc: Ray Ni
> > Cc: Star Zeng
> > Cc: Liming gao
> > Cc: Sean Brogan
> > Cc: Michael Turner
> > Cc: Bret Barkelew
> > Signed-off-by: Zhichao Gao
> > ---
> > .../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 00..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
> > +#include
> > +#include
> > +
> > +//
> > +// 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] LockEventTextDebug 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 a
Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
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
>
> 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
>
> (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
> >>
> >> 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
> >> Cc: Hao A Wu
> >> Cc: Ray Ni
> >> Cc: Star Zeng
> >> Cc: Liming gao
> >> Cc: Sean Brogan
> >> Cc: Michael Turner
> >> Cc: Bret Barkelew
> >> Signed-off-by: Zhichao Gao
> >> ---
> >> .../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 00..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
> >> +#include #include
> >> +
> >> +
> >> +//
> >> +// 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] LockEventTextDebug 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) {
>
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
>>
>> 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
>> Cc: Hao A Wu
>> Cc: Ray Ni
>> Cc: Star Zeng
>> Cc: Liming gao
>> Cc: Sean Brogan
>> Cc: Michael Turner
>> Cc: Bret Barkelew
>> Signed-off-by: Zhichao Gao
>> ---
>> .../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 00..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
>> +#include
>> +#include
>> +
>> +//
>> +// 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] LockEventTextDebug 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 00..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
[edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
From: Bret Barkelew
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
Cc: Hao A Wu
Cc: Ray Ni
Cc: Star Zeng
Cc: Liming gao
Cc: Sean Brogan
Cc: Michael Turner
Cc: Bret Barkelew
Signed-off-by: Zhichao Gao
---
.../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
diff --git
a/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c
b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c
new file mode 100644
index 00..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
+#include
+#include
+
+//
+// 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] LockEventTextDebug 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));
+ }
+}
diff --git
a/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
new file mode 100644
index 00..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
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44089): https://edk2.groups.io/g/devel/message/44089
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]
-=-=-=-=-=-=-=-=-=-=-=-
