From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 22 Jul 2019 13:45:14 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2D92F3082199; Mon, 22 Jul 2019 20:45:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-23.ams2.redhat.com [10.36.117.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id E96325D9DE; Mon, 22 Jul 2019 20:45:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance From: "Laszlo Ersek" To: devel@edk2.groups.io, zhichao.gao@intel.com Cc: Bret Barkelew , Jian J Wang , Hao A Wu , Ray Ni , Star Zeng , Liming gao , Sean Brogan , Michael Turner References: <20190722040204.33108-1-zhichao.gao@intel.com> <20190722040204.33108-3-zhichao.gao@intel.com> <13dd9cad-5328-c219-4222-10f90d30593c@redhat.com> Message-ID: Date: Mon, 22 Jul 2019 22:45:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <13dd9cad-5328-c219-4222-10f90d30593c@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Mon, 22 Jul 2019 20:45:14 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (apologies for the separate message, for this patch:) On 07/22/19 22:34, Laszlo Ersek wrote: > On 07/22/19 06:02, Gao, Zhichao wrote: >> From: Bret Barkelew >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2006 >> >> Add the instance of SecurityLockAuditLib. This instance >> has one interface SecurityLockReportEvent to log hardware >> and software security locks info. >> >> Cc: Jian J Wang >> Cc: Hao A Wu >> Cc: Ray Ni >> Cc: Star Zeng >> Cc: Liming gao >> Cc: Sean Brogan >> Cc: Michael Turner >> Cc: Bret Barkelew >> Signed-off-by: Zhichao Gao >> --- >> .../SecurityLockAuditDebugLib.c | 53 +++++++++++++++++++ >> .../SecurityLockAuditDebugLib.inf | 29 ++++++++++ >> 2 files changed, 82 insertions(+) >> create mode 100644 MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c >> create mode 100644 MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.inf The edk2 nomenclature usually puts the "implementation hints" of a library instance after the "Lib" word. For example, in "SecurityLockAuditLibNull", the word "Null" comes after "Lib" -- and that's correct. (From patch#3.) (2) In order to stick with this naming, I'd suggest renaming this lib instance to "SecurityLockAuditLibDebug". (Note that commit messages and subject lines should be updated too, not just code, and file names.) If the MdeModulePkg maintainers think the current name is OK, I won't insist on the rename. Thanks! Laszlo >> >> diff --git a/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c b/MdeModulePkg/Library/SecurityLockAuditDebugLib/SecurityLockAuditDebugLib.c >> new file mode 100644 >> index 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 >> +#include >> +#include >> + >> +// >> +// Used to look up lock name from LOCK_TYPE enum >> +// >> +CHAR8* mLockName[] = { >> + "SOFTWARE_LOCK", >> + "HARDWARE_LOCK" >> +}; >> + >> + >> +/** >> + Function for security Lock event logging and reporting >> + >> + @param[in] Module GUID of calling module >> + @param[in] Function Name of calling function >> + @param[in] 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 >> >