[PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event

[PATCH 0/5] Add new lib SecurityLockAuditLib to log message for security event

[Gao, Zhichao]

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

[PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib

[Gao, Zhichao]

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

[PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

[Gao, Zhichao]

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

[PATCH 3/5] MdeModulePkg/SecurityLockAuditLibNull: Add null version lib

[Gao, Zhichao]

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

[PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file

[Gao, Zhichao]

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

[PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug

[Gao, Zhichao]

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

Re: [edk2-devel] [PATCH 1/5] MdeModulePkg: Add header file for SecurityLockAuditLib

[Laszlo Ersek]

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
> 

Re: [edk2-devel] [PATCH 4/5] MdeModulePkg: Add SecuritAuditLib to dec file

[Laszlo Ersek]

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
> 

Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

[Laszlo Ersek]

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
> 

Re: [edk2-devel] [PATCH 5/5] MdeModulePkg/PiSmmIpl: Use SecurityLockAuditLib for debug

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

[Laszlo Ersek]

(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
>>
> 

Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

[Michael D Kinney]

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
> >>
> >
> 
> 
> 

Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance

[Wu, Hao A]

> -----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
> >