* [Patch 1/2] QuarkSocPkg/QncSmmDispatcher: Fix context passed to SMI handlers
2016-10-05 18:28 [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Michael Kinney
@ 2016-10-05 18:28 ` Michael Kinney
2016-10-05 18:28 ` [Patch 2/2] QuarkSocPkg/QncSmmDispatcher: Fix use after free issue Michael Kinney
2016-10-07 19:08 ` [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Steele, Kelly
2 siblings, 0 replies; 5+ messages in thread
From: Michael Kinney @ 2016-10-05 18:28 UTC (permalink / raw)
To: edk2-devel
https://bugzilla.tianocore.org/show_bug.cgi?id=136
1) Add CallbackContext field to the DATABASE_RECORD structure that
is set to the RegisterContent value passed to QNCSmmCoreRegister().
This is the content that must be passed to the SMI handler when
its source is triggered.
2) Update usage of ChildContext field in the DATABASE_RECOD to use
CopyMem() instead of structure assignments to avoid compiler
use of memcpy() intrinsics
This issue was reproduced using the unit test at:
https://github.com/mdkinney/edk2/tree/Bug51/Reproduce
An ASSERT() is generated the first time the periodic SMI
handler is triggered. After applying this patch, the
DEBUG() messages from the periodic SMI handler in this
unit test are generated.
Cc: Kelly Steele <kelly.steele@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
.../Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c | 4 ++--
.../QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h | 7 ++++---
.../QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c | 10 +++++-----
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c
index 1d1030c..670ca91 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c
@@ -1,7 +1,7 @@
/** @file
File to contain all the hardware specific stuff for the Periodical Timer dispatch protocol.
-Copyright (c) 2013-2015 Intel Corporation.
+Copyright (c) 2013-2016 Intel Corporation.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -177,7 +177,7 @@ PeriodicTimerGetContext (
// Update the elapsed time w/ the data from our tables
//
Record->CommBuffer.PeriodicTimer.ElapsedTime += TimerInterval->Interval;
- *HwContext = Record->ChildContext;
+ CopyMem (HwContext, &Record->ChildContext, sizeof (QNC_SMM_CONTEXT));
}
}
diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h
index 892294f..797be16 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h
@@ -1,7 +1,7 @@
/** @file
Prototypes and defines for the QNC SMM Dispatcher.
-Copyright (c) 2013-2015 Intel Corporation.
+Copyright (c) 2013-2016 Intel Corporation.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -396,8 +396,9 @@ struct _DATABASE_RECORD {
// Callback function
//
EFI_SMM_HANDLER_ENTRY_POINT2 Callback;
- QNC_SMM_CONTEXT ChildContext;
- QNC_SMM_BUFFER CommBuffer;
+ QNC_SMM_CONTEXT ChildContext;
+ VOID *CallbackContext;
+ QNC_SMM_BUFFER CommBuffer;
UINTN BufferSize;
//
diff --git a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c
index ba8c721..4783406 100644
--- a/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c
+++ b/QuarkSocPkg/QuarkNorthCluster/Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c
@@ -2,7 +2,7 @@
This driver is responsible for the registration of child drivers
and the abstraction of the QNC SMI sources.
-Copyright (c) 2013-2015 Intel Corporation.
+Copyright (c) 2013-2016 Intel Corporation.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
@@ -351,7 +351,8 @@ Returns:
// Gather information about the registration request
//
Record->Callback = DispatchFunction;
- Record->ChildContext = *RegisterContext;
+ Record->CallbackContext = RegisterContext;
+ CopyMem (&Record->ChildContext, RegisterContext, sizeof (QNC_SMM_CONTEXT));
Qualified = QUALIFIED_PROTOCOL_FROM_GENERIC (This);
@@ -407,7 +408,7 @@ Returns:
//
// Update ChildContext again as SwSmiInputValue has been changed
//
- Record->ChildContext = *RegisterContext;
+ CopyMem (&Record->ChildContext, RegisterContext, sizeof (QNC_SMM_CONTEXT));
}
//
@@ -688,7 +689,6 @@ QNCSmmCoreDispatcher (
// it supplied in registration. Simply pass back what it gave us.
//
ASSERT (RecordToExhaust->Callback != NULL);
- Context = RecordToExhaust->ChildContext;
ContextsMatch = TRUE;
}
@@ -710,7 +710,7 @@ QNCSmmCoreDispatcher (
RecordToExhaust->Callback (
(EFI_HANDLE) & RecordToExhaust->Link,
- &Context,
+ RecordToExhaust->CallbackContext,
CommunicationBuffer,
&BufferSize
);
--
2.6.3.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Patch 2/2] QuarkSocPkg/QncSmmDispatcher: Fix use after free issue
2016-10-05 18:28 [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Michael Kinney
2016-10-05 18:28 ` [Patch 1/2] QuarkSocPkg/QncSmmDispatcher: Fix context passed to SMI handlers Michael Kinney
@ 2016-10-05 18:28 ` Michael Kinney
2016-10-07 19:08 ` [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Steele, Kelly
2 siblings, 0 replies; 5+ messages in thread
From: Michael Kinney @ 2016-10-05 18:28 UTC (permalink / raw)
To: edk2-devel
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 <kelly.steele@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
---
.../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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs()
2016-10-05 18:28 [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Michael Kinney
2016-10-05 18:28 ` [Patch 1/2] QuarkSocPkg/QncSmmDispatcher: Fix context passed to SMI handlers Michael Kinney
2016-10-05 18:28 ` [Patch 2/2] QuarkSocPkg/QncSmmDispatcher: Fix use after free issue Michael Kinney
@ 2016-10-07 19:08 ` Steele, Kelly
2016-10-07 21:02 ` Steele, Kelly
2 siblings, 1 reply; 5+ messages in thread
From: Steele, Kelly @ 2016-10-07 19:08 UTC (permalink / raw)
To: Kinney, Michael D, edk2-devel@lists.01.org
Reviewed-by: Kelly Steele <kelly.steele@intel.com>
> -----Original Message-----
> From: Kinney, Michael D
> Sent: October 05, 2016 11:29
> To: edk2-devel@lists.01.org
> Cc: Steele, Kelly <kelly.steele@intel.com>
> Subject: [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler
> ASSERTs()
>
> This series fixes the following two issues:
>
> QuarkSocPkg QncSmmDispatcher passes incorrect context to SMI handler
> https://bugzilla.tianocore.org/show_bug.cgi?id=136
>
> QuarkSockg Use after free in QNCSmmCoreDispatcher
> https://bugzilla.tianocore.org/show_bug.cgi?id=51
>
> These issues can be reproduced using the unit test available in the following
> branch that registers a periodic SMI that is triggered every 8 seconds and
> unregisters itself after the periodic SMI handler has been triggered 4 times.
>
> https://github.com/mdkinney/edk2/tree/Bug51/Reproduce
>
> Cc: Kelly Steele <kelly.steele@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
>
> Michael Kinney (2):
> QuarkSocPkg/QncSmmDispatcher: Fix context passed to SMI handlers
> QuarkSocPkg/QncSmmDispatcher: Fix use after free issue
>
> .../QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c | 4 +-
> .../Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h | 9 ++--
> .../Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c | 51
> +++++++++++++++++-----
> 3 files changed, 49 insertions(+), 15 deletions(-)
>
> --
> 2.6.3.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs()
2016-10-07 19:08 ` [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler ASSERTs() Steele, Kelly
@ 2016-10-07 21:02 ` Steele, Kelly
0 siblings, 0 replies; 5+ messages in thread
From: Steele, Kelly @ 2016-10-07 21:02 UTC (permalink / raw)
To: Kinney, Michael D, 'edk2-devel@lists.01.org'
Entire patch reviewed. Looks good to me.
Reviewed-by: Kelly Steele <kelly.steele@intel.com>
> -----Original Message-----
> From: Steele, Kelly
> Sent: October 07, 2016 12:09
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler
> ASSERTs()
>
>
>
> Reviewed-by: Kelly Steele <kelly.steele@intel.com>
>
>
>
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: October 05, 2016 11:29
> > To: edk2-devel@lists.01.org
> > Cc: Steele, Kelly <kelly.steele@intel.com>
> > Subject: [Patch 0/2] QuarkSocPkg/QncSmmDispatcher: Fix SMI Handler
> > ASSERTs()
> >
> > This series fixes the following two issues:
> >
> > QuarkSocPkg QncSmmDispatcher passes incorrect context to SMI handler
> > https://bugzilla.tianocore.org/show_bug.cgi?id=136
> >
> > QuarkSockg Use after free in QNCSmmCoreDispatcher
> > https://bugzilla.tianocore.org/show_bug.cgi?id=51
> >
> > These issues can be reproduced using the unit test available in the following
> > branch that registers a periodic SMI that is triggered every 8 seconds and
> > unregisters itself after the periodic SMI handler has been triggered 4 times.
> >
> > https://github.com/mdkinney/edk2/tree/Bug51/Reproduce
> >
> > Cc: Kelly Steele <kelly.steele@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Michael Kinney <michael.d.kinney@intel.com>
> >
> > Michael Kinney (2):
> > QuarkSocPkg/QncSmmDispatcher: Fix context passed to SMI handlers
> > QuarkSocPkg/QncSmmDispatcher: Fix use after free issue
> >
> > .../QncSmmDispatcher/QNC/QNCSmmPeriodicTimer.c | 4 +-
> > .../Smm/DxeSmm/QncSmmDispatcher/QNCSmm.h | 9 ++--
> > .../Smm/DxeSmm/QncSmmDispatcher/QNCSmmCore.c | 51
> > +++++++++++++++++-----
> > 3 files changed, 49 insertions(+), 15 deletions(-)
> >
> > --
> > 2.6.3.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread