* [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event
@ 2019-07-22 4:01 Gao, Zhichao
2019-07-22 4:02 ` [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib Gao, Zhichao
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:01 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
For clearly see where security events occur in the platform.
Add SecurityLockAuditLib to provide a consistant debug log
message format that can be easily parsed in a post processing step.
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>
Bret Barkelew (5):
MdeModulePkg: Add header file for SecurityLockAuditLib
MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
MdeModulePkg/SecurityLockAuditLibNull: Add null version lib
MdeModulePkg: Add SecuritAuditLib to dec file
MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 +
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
.../Include/Library/SecurityLockAuditLib.h | 47 ++++++++++++++++
.../SecurityLockAuditDebugLib.c | 53 +++++++++++++++++++
.../SecurityLockAuditDebugLib.inf | 29 ++++++++++
.../SecurityLockAuditLibNull.c | 32 +++++++++++
.../SecurityLockAuditLibNull.inf | 27 ++++++++++
MdeModulePkg/MdeModulePkg.dec | 4 ++
MdeModulePkg/MdeModulePkg.dsc | 3 ++
9 files changed, 198 insertions(+)
create mode 100644 MdeModulePkg/Include/Library/SecurityLockAuditLib.h
create mode 100644 MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c
create mode 100644 MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
create mode 100644 MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.c
create mode 100644 MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
@ 2019-07-22 4:02 ` Gao, Zhichao
2019-07-22 20:27 ` [edk2-devel] " Laszlo Ersek
2019-07-22 4:02 ` [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance Gao, Zhichao
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:02 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
Add header file for SecurityLockAuditLib and add its
file path to dec file.
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>
---
.../Include/Library/SecurityLockAuditLib.h | 47 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 4 ++
2 files changed, 51 insertions(+)
create mode 100644 MdeModulePkg/Include/Library/SecurityLockAuditLib.h
diff --git a/MdeModulePkg/Include/Library/SecurityLockAuditLib.h b/MdeModulePkg/Include/Library/SecurityLockAuditLib.h
new file mode 100644
index 0000000000..db3b145aba
--- /dev/null
+++ b/MdeModulePkg/Include/Library/SecurityLockAuditLib.h
@@ -0,0 +1,47 @@
+/** @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
+
+**/
+
+#ifndef __SECURITY_LOCK_LIB_H__
+#define __SECURITY_LOCK_LIB_H__
+
+
+#define SECURITY_LOCK_REPORT_EVENT(LockMessage,LockType) \
+ SecurityLockReportEvent (&gEfiCallerIdGuid, __FUNCTION__, LockMessage, LockType);
+
+/**
+ Enum to hold the various lock types for use in post-processing
+
+**/
+typedef enum {
+ SOFTWARE_LOCK = 0,
+ HARDWARE_LOCK,
+} LOCK_TYPE;
+
+
+/**
+ 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 Event text 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
+ );
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 12e0bbf579..ee2828dd8e 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -153,6 +153,10 @@
#
DisplayUpdateProgressLib|Include/Library/DisplayUpdateProgressLib.h
+ ## @libraryclass Provides a way for logging security locks
+ #
+ SecurityLockAuditLib|Include/Library/SecurityLockAuditLib.h
+
[Guids]
## MdeModule package token space guid
# Include/Guid/MdeModulePkgTokenSpace.h
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
2019-07-22 4:02 ` [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib Gao, Zhichao
@ 2019-07-22 4:02 ` Gao, Zhichao
2019-07-22 20:34 ` [edk2-devel] " Laszlo Ersek
2019-07-22 4:02 ` [PATCH 3/5] MdeModulePkg/SecurityLockAuditLibNull: Add null version lib Gao, Zhichao
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:02 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
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
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));
+ }
+}
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
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] MdeModulePkg/SecurityLockAuditLibNull: Add null version lib
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
2019-07-22 4:02 ` [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib Gao, Zhichao
2019-07-22 4:02 ` [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance Gao, Zhichao
@ 2019-07-22 4:02 ` Gao, Zhichao
2019-07-22 4:02 ` [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file Gao, Zhichao
2019-07-22 4:02 ` [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug Gao, Zhichao
4 siblings, 0 replies; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:02 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
Add a null version library instance of SecurityLockAuditLib.
It provides the new API SecurityLockReportEvent without any
function.
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>
---
.../SecurityLockAuditLibNull.c | 32 +++++++++++++++++++
.../SecurityLockAuditLibNull.inf | 27 ++++++++++++++++
2 files changed, 59 insertions(+)
create mode 100644 MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.c
create mode 100644 MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
diff --git a/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.c b/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.c
new file mode 100644
index 0000000000..47a26684d3
--- /dev/null
+++ b/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.c
@@ -0,0 +1,32 @@
+/** @file
+
+ Null library for security lock logging that does nothing but meet compile requirements
+
+ Copyright (c) 2018, Microsoft Corporation
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Library/SecurityLockAuditLib.h>
+
+/**
+ Null 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 Event text 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
+ )
+{
+}
diff --git a/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf b/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
new file mode 100644
index 0000000000..bf3f9fc0b0
--- /dev/null
+++ b/MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
@@ -0,0 +1,27 @@
+## @file
+#
+# Null library for security lock logging that does nothing but meet compile requirements
+#
+#
+# Copyright (c) 2018, Microsoft Corporation
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = SecurityLockAuditLibNull
+ FILE_GUID = 1d333a6a-90a7-45cb-9897-0a172ee35066
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = SecurityLockAuditLib
+
+[Sources.common]
+ SecurityLockAuditLibNull.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+
+[LibraryClasses]
+ BaseLib
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
` (2 preceding siblings ...)
2019-07-22 4:02 ` [PATCH 3/5] MdeModulePkg/SecurityLockAuditLibNull: Add null version lib Gao, Zhichao
@ 2019-07-22 4:02 ` Gao, Zhichao
2019-07-22 20:27 ` [edk2-devel] " Laszlo Ersek
2019-07-22 4:02 ` [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug Gao, Zhichao
4 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:02 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
Add the lib instance to LibraryClasses for components
that consume it.
Add the lib instance to Components for build only.
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>
---
MdeModulePkg/MdeModulePkg.dsc | 3 +++
1 file changed, 3 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 6ca7d9ade4..f3d8ffb5c6 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -27,6 +27,7 @@
DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
+ SecurityLockAuditLib|MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
#
# Basic
#
@@ -271,6 +272,8 @@
MdeModulePkg/Core/Pei/PeiMain.inf
MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
+ MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
+ MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
MdeModulePkg/Library/UefiMemoryAllocationProfileLib/UefiMemoryAllocationProfileLib.inf
MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
` (3 preceding siblings ...)
2019-07-22 4:02 ` [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file Gao, Zhichao
@ 2019-07-22 4:02 ` Gao, Zhichao
2019-07-22 20:40 ` [edk2-devel] " Laszlo Ersek
4 siblings, 1 reply; 13+ messages in thread
From: Gao, Zhichao @ 2019-07-22 4:02 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006
Use SecurityLockAuditLib in PiSmmIpl to output debug message
while lock the SMRAM.
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>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 ++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
2 files changed, 3 insertions(+)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 1cf8c93227..604eb1b98e 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -33,6 +33,7 @@
#include <Library/UefiRuntimeLib.h>
#include <Library/PcdLib.h>
#include <Library/ReportStatusCodeLib.h>
+#include <Library/SecurityLockAuditLib.h>
#include "PiSmmCorePrivateData.h"
@@ -780,6 +781,7 @@ SmmIplReadyToLockEventNotify (
// Lock the SMRAM (Note: Locking SMRAM may not be supported on all platforms)
//
mSmmAccess->Lock (mSmmAccess);
+ SECURITY_LOCK_REPORT_EVENT ("Lock SMRAM", HARDWARE_LOCK);
//
// Close protocol and event notification events that do not apply after the
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index b6b1bbcdac..2240ab3c5f 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@
DxeServicesLib
PcdLib
ReportStatusCodeLib
+ SecurityLockAuditLib
[Protocols]
gEfiSmmBase2ProtocolGuid ## PRODUCES
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib
2019-07-22 4:02 ` [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib Gao, Zhichao
@ 2019-07-22 20:27 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2019-07-22 20:27 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
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 header file for SecurityLockAuditLib and add its
> file path to dec file.
>
> 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>
> ---
> .../Include/Library/SecurityLockAuditLib.h | 47 +++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 4 ++
> 2 files changed, 51 insertions(+)
> create mode 100644 MdeModulePkg/Include/Library/SecurityLockAuditLib.h
>
> diff --git a/MdeModulePkg/Include/Library/SecurityLockAuditLib.h b/MdeModulePkg/Include/Library/SecurityLockAuditLib.h
> new file mode 100644
> index 0000000000..db3b145aba
> --- /dev/null
> +++ b/MdeModulePkg/Include/Library/SecurityLockAuditLib.h
> @@ -0,0 +1,47 @@
> +/** @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
> +
> +**/
> +
> +#ifndef __SECURITY_LOCK_LIB_H__
> +#define __SECURITY_LOCK_LIB_H__
> +
> +
> +#define SECURITY_LOCK_REPORT_EVENT(LockMessage,LockType) \
> + SecurityLockReportEvent (&gEfiCallerIdGuid, __FUNCTION__, LockMessage, LockType);
> +
> +/**
> + Enum to hold the various lock types for use in post-processing
> +
> +**/
> +typedef enum {
> + SOFTWARE_LOCK = 0,
> + HARDWARE_LOCK,
> +} LOCK_TYPE;
> +
> +
> +/**
> + 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 Event text 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
> + );
> +
> +#endif
(1) Has support for conversion specifiers (i.e., a format string) been
considered?
Thanks!
Laszlo
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 12e0bbf579..ee2828dd8e 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -153,6 +153,10 @@
> #
> DisplayUpdateProgressLib|Include/Library/DisplayUpdateProgressLib.h
>
> + ## @libraryclass Provides a way for logging security locks
> + #
> + SecurityLockAuditLib|Include/Library/SecurityLockAuditLib.h
> +
> [Guids]
> ## MdeModule package token space guid
> # Include/Guid/MdeModulePkgTokenSpace.h
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file
2019-07-22 4:02 ` [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file Gao, Zhichao
@ 2019-07-22 20:27 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2019-07-22 20:27 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
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 lib instance to LibraryClasses for components
> that consume it.
> Add the lib instance to Components for build only.
>
> 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>
> ---
> MdeModulePkg/MdeModulePkg.dsc | 3 +++
> 1 file changed, 3 insertions(+)
(1) The subject says "dec" file, but the patch modifies the DSC file.
Thanks
Laszlo
>
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index 6ca7d9ade4..f3d8ffb5c6 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -27,6 +27,7 @@
> DxeCoreEntryPoint|MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
> UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
> + SecurityLockAuditLib|MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
> #
> # Basic
> #
> @@ -271,6 +272,8 @@
> MdeModulePkg/Core/Pei/PeiMain.inf
> MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf
>
> + MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf
> + MdeModulePkg/Library/SecurityLockAuditLibNull/SecurityLockAuditLibNull.inf
> MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> MdeModulePkg/Library/UefiMemoryAllocationProfileLib/UefiMemoryAllocationProfileLib.inf
> MdeModulePkg/Library/DxeCoreMemoryAllocationLib/DxeCoreMemoryAllocationLib.inf
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
2019-07-22 4:02 ` [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance Gao, Zhichao
@ 2019-07-22 20:34 ` Laszlo Ersek
2019-07-22 20:45 ` Laszlo Ersek
2019-07-23 3:26 ` Wu, Hao A
0 siblings, 2 replies; 13+ messages in thread
From: Laszlo Ersek @ 2019-07-22 20:34 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
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
>
> 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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug
2019-07-22 4:02 ` [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug Gao, Zhichao
@ 2019-07-22 20:40 ` Laszlo Ersek
0 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2019-07-22 20:40 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
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
>
> Use SecurityLockAuditLib in PiSmmIpl to output debug message
> while lock the SMRAM.
>
> 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>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 2 ++
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 1cf8c93227..604eb1b98e 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -33,6 +33,7 @@
> #include <Library/UefiRuntimeLib.h>
> #include <Library/PcdLib.h>
> #include <Library/ReportStatusCodeLib.h>
> +#include <Library/SecurityLockAuditLib.h>
>
> #include "PiSmmCorePrivateData.h"
>
> @@ -780,6 +781,7 @@ SmmIplReadyToLockEventNotify (
> // Lock the SMRAM (Note: Locking SMRAM may not be supported on all platforms)
> //
> mSmmAccess->Lock (mSmmAccess);
> + SECURITY_LOCK_REPORT_EVENT ("Lock SMRAM", HARDWARE_LOCK);
>
> //
> // Close protocol and event notification events that do not apply after the
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> index b6b1bbcdac..2240ab3c5f 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> @@ -46,6 +46,7 @@
> DxeServicesLib
> PcdLib
> ReportStatusCodeLib
> + SecurityLockAuditLib
>
> [Protocols]
> gEfiSmmBase2ProtocolGuid ## PRODUCES
>
Here a new lib class dependency is being introduced to "PiSmmIpl.inf".
In addition, the new lib class itself is introduced as a new feature, to
"MdeModulePkg.dec", in patch #1.
This means that every platform DSC that currently consumes
"PiSmmIpl.inf" will fail to build, after this patch set is merged.
That is fine for platform DSCs that live outside of the edk2 repository.
It is not fine for platform DSCs that live inside edk2.
Whenever implementing such patches, please always grep the entire edk2
repo for matches, and implement the necessary updates (you can of course
ask for details before submitting v1).
In the present case, we have
$ git grep -F PiSmmIpl.inf -- '*dsc*'
MdeModulePkg/MdeModulePkg.dsc: MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
OvmfPkg/OvmfPkgIa32.dsc: MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
OvmfPkg/OvmfPkgIa32X64.dsc: MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
OvmfPkg/OvmfPkgX64.dsc: MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
MdeModulePkg.dsc is updated in patch #4.
(1) Thus, please *prepend* a patch to patch#5, for OvmfPkg:
In the [LibraryClasses] section of all three DSC files, please resolve
the SecurityLockAuditLib class to the SecurityLockAuditDebugLib instance.
Doing things in this order will keep the tree bisectable -- at no stage
of the patch series will OVMF fail to build.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
2019-07-22 20:34 ` [edk2-devel] " Laszlo Ersek
@ 2019-07-22 20:45 ` Laszlo Ersek
2019-07-22 21:06 ` Michael D Kinney
2019-07-23 3:26 ` Wu, Hao A
1 sibling, 1 reply; 13+ messages in thread
From: Laszlo Ersek @ 2019-07-22 20:45 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Bret Barkelew, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng,
Liming gao, Sean Brogan, Michael Turner
(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
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
2019-07-22 20:45 ` Laszlo Ersek
@ 2019-07-22 21:06 ` Michael D Kinney
0 siblings, 0 replies; 13+ messages in thread
From: Michael D Kinney @ 2019-07-22 21:06 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Gao, Zhichao,
Kinney, Michael D
Cc: Bret Barkelew, Wang, Jian J, Wu, Hao A, Ni, Ray, Zeng, Star,
Gao, Liming, Sean Brogan, Michael Turner
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.gao@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@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@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/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
> >>
> >
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance
2019-07-22 20:34 ` [edk2-devel] " Laszlo Ersek
2019-07-22 20:45 ` Laszlo Ersek
@ 2019-07-23 3:26 ` Wu, Hao A
1 sibling, 0 replies; 13+ messages in thread
From: Wu, Hao A @ 2019-07-23 3:26 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Gao, Zhichao
Cc: Bret Barkelew, Wang, Jian J, Ni, Ray, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@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.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/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
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-07-23 3:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-22 4:01 [PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event Gao, Zhichao
2019-07-22 4:02 ` [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib Gao, Zhichao
2019-07-22 20:27 ` [edk2-devel] " Laszlo Ersek
2019-07-22 4:02 ` [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance Gao, Zhichao
2019-07-22 20:34 ` [edk2-devel] " Laszlo Ersek
2019-07-22 20:45 ` Laszlo Ersek
2019-07-22 21:06 ` Michael D Kinney
2019-07-23 3:26 ` Wu, Hao A
2019-07-22 4:02 ` [PATCH 3/5] MdeModulePkg/SecurityLockAuditLibNull: Add null version lib Gao, Zhichao
2019-07-22 4:02 ` [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file Gao, Zhichao
2019-07-22 20:27 ` [edk2-devel] " Laszlo Ersek
2019-07-22 4:02 ` [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug Gao, Zhichao
2019-07-22 20:40 ` [edk2-devel] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox