From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7F7CD1A1EEA for ; Wed, 5 Oct 2016 11:28:57 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 05 Oct 2016 11:28:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,302,1473145200"; d="scan'208";a="1060913938" Received: from mdkinney-mobl.amr.corp.intel.com ([10.232.96.21]) by orsmga002.jf.intel.com with ESMTP; 05 Oct 2016 11:28:57 -0700 From: Michael Kinney To: edk2-devel@lists.01.org Date: Wed, 5 Oct 2016 11:28:50 -0700 Message-Id: <1475692130-20756-3-git-send-email-michael.d.kinney@intel.com> X-Mailer: git-send-email 2.6.3.windows.1 In-Reply-To: <1475692130-20756-1-git-send-email-michael.d.kinney@intel.com> References: <1475692130-20756-1-git-send-email-michael.d.kinney@intel.com> Subject: [Patch 2/2] QuarkSocPkg/QncSmmDispatcher: Fix use after free issue X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Oct 2016 18:28:57 -0000 Update Quark North Cluster SMM dispatcher logic to handle case where an SMI handler unregisters itself. https://bugzilla.tianocore.org/show_bug.cgi?id=51 This issue was reproduced using the unit test at: https://github.com/mdkinney/edk2/tree/Bug51/Reproduce An ASSERT() is generated the 4th time the periodic SMI handler is triggered when the periodic SMI handler unregisters itself. After applying this patch, the DEBUG() message from the periodic SMI handler in this unit test is generated 4 times, the periodic SMI handler is unregistered, and the UEFI Shell works as expected. Cc: Kelly Steele Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Michael Kinney --- .../Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h | 2 ++ .../Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c | 41 +++++++++++++++++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h index 797be16..ccf9dae 100644 --- a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h +++ b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h @@ -387,6 +387,8 @@ struct _DATABASE_RECORD { UINT32 Signature; LIST_ENTRY Link; + BOOLEAN Processed; + // // Status and Enable bit description // diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c index 4783406..c2f75f8 100644 --- a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c +++ b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c @@ -592,6 +592,7 @@ QNCSmmCoreDispatcher ( BOOLEAN ChildWasDispatched; DATABASE_RECORD *RecordInDb; + DATABASE_RECORD ActiveRecordInDb; LIST_ENTRY *LinkInDb; DATABASE_RECORD *RecordToExhaust; LIST_ENTRY *LinkToExhaust; @@ -614,6 +615,16 @@ QNCSmmCoreDispatcher ( ChildWasDispatched = FALSE; // + // Mark all child handlers as not processed + // + LinkInDb = GetFirstNode (&mPrivateData.CallbackDataBase); + while (!IsNull (&mPrivateData.CallbackDataBase, LinkInDb)) { + RecordInDb = DATABASE_RECORD_FROM_LINK (LinkInDb); + RecordInDb->Processed = FALSE; + LinkInDb = GetNextNode (&mPrivateData.CallbackDataBase, LinkInDb); + } + + // // Preserve Index registers // SaveState (); @@ -634,6 +645,12 @@ QNCSmmCoreDispatcher ( while ((!IsNull (&mPrivateData.CallbackDataBase, LinkInDb)) && (ResetListSearch == FALSE)) { RecordInDb = DATABASE_RECORD_FROM_LINK (LinkInDb); + // + // Make a copy of the record that contains an active SMI source, + // because un-register maybe invoked in callback function and + // RecordInDb maybe released + // + CopyMem (&ActiveRecordInDb, RecordInDb, sizeof (ActiveRecordInDb)); // // look for the first active source @@ -663,6 +680,13 @@ QNCSmmCoreDispatcher ( // while (!IsNull (&mPrivateData.CallbackDataBase, LinkToExhaust)) { RecordToExhaust = DATABASE_RECORD_FROM_LINK (LinkToExhaust); + LinkToExhaust = GetNextNode (&mPrivateData.CallbackDataBase, LinkToExhaust); + if (RecordToExhaust->Processed) { + // + // Record has already been processed. Continue with next child handler. + // + continue; + } if (CompareSources (&RecordToExhaust->SrcDesc, &ActiveSource)) { // @@ -692,6 +716,11 @@ QNCSmmCoreDispatcher ( ContextsMatch = TRUE; } + // + // Mark this child handler so it will not be processed again + // + RecordToExhaust->Processed = TRUE; + if (ContextsMatch) { if (RecordToExhaust->BufferSize != 0) { @@ -720,11 +749,13 @@ QNCSmmCoreDispatcher ( SxChildWasDispatched = TRUE; } } + // + // Can not use RecordInDb after this point because Callback may have unregistered RecordInDb + // Restart processing of SMI handlers from the begining of the linked list because the + // state of the linked listed may have been modified due to unregister actions in the Callback. + // + LinkToExhaust = GetFirstNode (&mPrivateData.CallbackDataBase); } - // - // Get next record in DB - // - LinkToExhaust = GetNextNode (&mPrivateData.CallbackDataBase, &RecordToExhaust->Link); } if (RecordInDb->ClearSource == NULL) { @@ -736,7 +767,7 @@ QNCSmmCoreDispatcher ( // // This source requires special handling to clear // - RecordInDb->ClearSource (&ActiveSource); + ActiveRecordInDb.ClearSource (&ActiveSource); } if (ChildWasDispatched) { -- 2.6.3.windows.1