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:34:04 -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 DEF2A308FC20; Mon, 22 Jul 2019 20:34:03 +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 A2A745D9D3; Mon, 22 Jul 2019 20:34:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SecurityLockAuditDebugLib: Add lib instance 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> From: "Laszlo Ersek" Message-ID: <13dd9cad-5328-c219-4222-10f90d30593c@redhat.com> Date: Mon, 22 Jul 2019 22:34:00 +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: <20190722040204.33108-3-zhichao.gao@intel.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.43]); Mon, 22 Jul 2019 20:34:04 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > > 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 >