[edk2-devel] [PATCH v2 0/4] Support to unregister SMI handler inside SMI handler

[edk2-devel] [PATCH v2 0/4] Support to unregister SMI handler inside SMI handler

[Zhiguang Liu]

This patch set is to support to unregister SMI handler inside SMI handler,
also add check to not allow unregister SMI handler in other SMI handler.
This patch set also have the same logic in StandaloneMmPkg.
Because no change on the first patch, I kept the R-B for it.

Zhiguang Liu (4):
  MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler
  MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler
  StandaloneMmPkg: Support to unregister MMI handler inside MMI handler
  StandaloneMmPkg: Disallow unregister MMI handler in other MMI handler

 MdeModulePkg/Core/PiSmmCore/Smi.c | 40 ++++++++++++++++++++++-------
 StandaloneMmPkg/Core/Mmi.c        | 42 +++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 19 deletions(-)

-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116071): https://edk2.groups.io/g/devel/message/116071
Mute This Topic: https://groups.io/mt/104616991/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/4] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler

[Zhiguang Liu]

To support unregister SMI handler inside SMI handler itself,
get next node before SMI handler is executed, since LIST_ENTRY that
Link points to may be freed if unregister SMI handler in SMI handler
itself.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 2985f989c3..3489c130fd 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -134,8 +134,14 @@ SmiManage (
 
   Head = &SmiEntry->SmiHandlers;
 
-  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
+  for (Link = Head->ForwardLink; Link != Head;) {
     SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
+    //
+    // To support unregister SMI handler inside SMI handler itself,
+    // get next node before handler is executed, since LIST_ENTRY that
+    // Link points to may be freed if unregister SMI handler.
+    //
+    Link = Link->ForwardLink;
 
     Status = SmiHandler->Handler (
                            (EFI_HANDLE)SmiHandler,
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116072): https://edk2.groups.io/g/devel/message/116072
Mute This Topic: https://groups.io/mt/104616992/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/4] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, February 28, 2024 10:28 AM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH v2 1/4] MdeModulePkg/SMM: Support to unregister SMI
> handler inside SMI handler
> 
> To support unregister SMI handler inside SMI handler itself,
> get next node before SMI handler is executed, since LIST_ENTRY that
> Link points to may be freed if unregister SMI handler in SMI handler
> itself.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/Smi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c
> b/MdeModulePkg/Core/PiSmmCore/Smi.c
> index 2985f989c3..3489c130fd 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> @@ -134,8 +134,14 @@ SmiManage (
> 
>    Head = &SmiEntry->SmiHandlers;
> 
> -  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
> +  for (Link = Head->ForwardLink; Link != Head;) {
>      SmiHandler = CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE);
> +    //
> +    // To support unregister SMI handler inside SMI handler itself,
> +    // get next node before handler is executed, since LIST_ENTRY that
> +    // Link points to may be freed if unregister SMI handler.
> +    //
> +    Link = Link->ForwardLink;
> 
>      Status = SmiHandler->Handler (
>                             (EFI_HANDLE)SmiHandler,
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116164): https://edk2.groups.io/g/devel/message/116164
Mute This Topic: https://groups.io/mt/104616992/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/4] MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler

[Zhiguang Liu]

In last patch, we add code support to unregister SMI handler inside
itself. However, the code doesn't support unregister SMI handler
insider other SMI handler. While this is not a must-have usage.
So add check to disallow unregister SMI handler in other SMI handler.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/Smi.c | 32 +++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
index 3489c130fd..1bfbc635fc 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,7 +8,8 @@
 
 #include "PiSmmCore.h"
 
-LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
+SMI_HANDLER  *gCurrentSmiHandler = NULL;
+LIST_ENTRY   mSmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
 
 SMI_ENTRY  mRootSmiEntry = {
   SMI_ENTRY_SIGNATURE,
@@ -142,13 +143,18 @@ SmiManage (
     // Link points to may be freed if unregister SMI handler.
     //
     Link = Link->ForwardLink;
-
-    Status = SmiHandler->Handler (
-                           (EFI_HANDLE)SmiHandler,
-                           Context,
-                           CommBuffer,
-                           CommBufferSize
-                           );
+    //
+    // Assign gCurrentSmiHandle before calling the SMI handler and
+    // set to NULL when it returns.
+    //
+    gCurrentSmiHandler = SmiHandler;
+    Status             = SmiHandler->Handler (
+                                       (EFI_HANDLE)SmiHandler,
+                                       Context,
+                                       CommBuffer,
+                                       CommBufferSize
+                                       );
+    gCurrentSmiHandler = NULL;
 
     switch (Status) {
       case EFI_INTERRUPT_PENDING:
@@ -328,6 +334,16 @@ SmiHandlerUnRegister (
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Check if unregister SMI handler inside a SMI Handler
+  //
+  if (gCurrentSmiHandler != NULL) {
+    //
+    // Only allow to unregister SMI Handler inside itself.
+    //
+    ASSERT (gCurrentSmiHandler == SmiHandler);
+  }
+
   SmiEntry = SmiHandler->SmiEntry;
 
   RemoveEntryList (&SmiHandler->Link);
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116073): https://edk2.groups.io/g/devel/message/116073
Mute This Topic: https://groups.io/mt/104616993/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/4] MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler

[Laszlo Ersek]

On 2/28/24 03:27, Zhiguang Liu wrote:
> In last patch, we add code support to unregister SMI handler inside
> itself. However, the code doesn't support unregister SMI handler
> insider other SMI handler. While this is not a must-have usage.
> So add check to disallow unregister SMI handler in other SMI handler.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Core/PiSmmCore/Smi.c | 32 +++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCore/Smi.c
> index 3489c130fd..1bfbc635fc 100644
> --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> @@ -8,7 +8,8 @@
>  
>  #include "PiSmmCore.h"
>  
> -LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
> +SMI_HANDLER  *gCurrentSmiHandler = NULL;
> +LIST_ENTRY   mSmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList);
>  
>  SMI_ENTRY  mRootSmiEntry = {
>    SMI_ENTRY_SIGNATURE,
> @@ -142,13 +143,18 @@ SmiManage (
>      // Link points to may be freed if unregister SMI handler.
>      //
>      Link = Link->ForwardLink;
> -
> -    Status = SmiHandler->Handler (
> -                           (EFI_HANDLE)SmiHandler,
> -                           Context,
> -                           CommBuffer,
> -                           CommBufferSize
> -                           );
> +    //
> +    // Assign gCurrentSmiHandle before calling the SMI handler and
> +    // set to NULL when it returns.
> +    //
> +    gCurrentSmiHandler = SmiHandler;
> +    Status             = SmiHandler->Handler (
> +                                       (EFI_HANDLE)SmiHandler,
> +                                       Context,
> +                                       CommBuffer,
> +                                       CommBufferSize
> +                                       );
> +    gCurrentSmiHandler = NULL;
>  
>      switch (Status) {
>        case EFI_INTERRUPT_PENDING:
> @@ -328,6 +334,16 @@ SmiHandlerUnRegister (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Check if unregister SMI handler inside a SMI Handler
> +  //
> +  if (gCurrentSmiHandler != NULL) {
> +    //
> +    // Only allow to unregister SMI Handler inside itself.
> +    //
> +    ASSERT (gCurrentSmiHandler == SmiHandler);
> +  }
> +
>    SmiEntry = SmiHandler->SmiEntry;
>  
>    RemoveEntryList (&SmiHandler->Link);

(1) Why not:

  if ((gCurrentSmiHandler != NULL) && (gCurrentSmiHandler != SmiHandler)) {
    return EFI_INVALID_PARAMETER;
  }

?

(2) Why do we call the new global variable "gCurrentSmiHandler" rather than "mCurrentSmiHandler"?

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116101): https://edk2.groups.io/g/devel/message/116101
Mute This Topic: https://groups.io/mt/104616993/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/4] MdeModulePkg/SMM: Disallow unregister SMI handler in other SMI handler

[Zhiguang Liu]

Thanks Laszlo. Both are good comments. I will follow in next version

Thanks
Zhiguang

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 28, 2024 4:45 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 2/4] MdeModulePkg/SMM: Disallow
> unregister SMI handler in other SMI handler
> 
> On 2/28/24 03:27, Zhiguang Liu wrote:
> > In last patch, we add code support to unregister SMI handler inside
> > itself. However, the code doesn't support unregister SMI handler
> > insider other SMI handler. While this is not a must-have usage.
> > So add check to disallow unregister SMI handler in other SMI handler.
> >
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >  MdeModulePkg/Core/PiSmmCore/Smi.c | 32
> > +++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c
> > b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > index 3489c130fd..1bfbc635fc 100644
> > --- a/MdeModulePkg/Core/PiSmmCore/Smi.c
> > +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
> > @@ -8,7 +8,8 @@
> >
> >  #include "PiSmmCore.h"
> >
> > -LIST_ENTRY  mSmiEntryList = INITIALIZE_LIST_HEAD_VARIABLE
> > (mSmiEntryList);
> > +SMI_HANDLER  *gCurrentSmiHandler = NULL;
> > +LIST_ENTRY   mSmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE
> (mSmiEntryList);
> >
> >  SMI_ENTRY  mRootSmiEntry = {
> >    SMI_ENTRY_SIGNATURE,
> > @@ -142,13 +143,18 @@ SmiManage (
> >      // Link points to may be freed if unregister SMI handler.
> >      //
> >      Link = Link->ForwardLink;
> > -
> > -    Status = SmiHandler->Handler (
> > -                           (EFI_HANDLE)SmiHandler,
> > -                           Context,
> > -                           CommBuffer,
> > -                           CommBufferSize
> > -                           );
> > +    //
> > +    // Assign gCurrentSmiHandle before calling the SMI handler and
> > +    // set to NULL when it returns.
> > +    //
> > +    gCurrentSmiHandler = SmiHandler;
> > +    Status             = SmiHandler->Handler (
> > +                                       (EFI_HANDLE)SmiHandler,
> > +                                       Context,
> > +                                       CommBuffer,
> > +                                       CommBufferSize
> > +                                       );
> > +    gCurrentSmiHandler = NULL;
> >
> >      switch (Status) {
> >        case EFI_INTERRUPT_PENDING:
> > @@ -328,6 +334,16 @@ SmiHandlerUnRegister (
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > +  //
> > +  // Check if unregister SMI handler inside a SMI Handler  //  if
> > + (gCurrentSmiHandler != NULL) {
> > +    //
> > +    // Only allow to unregister SMI Handler inside itself.
> > +    //
> > +    ASSERT (gCurrentSmiHandler == SmiHandler);  }
> > +
> >    SmiEntry = SmiHandler->SmiEntry;
> >
> >    RemoveEntryList (&SmiHandler->Link);
> 
> (1) Why not:
> 
>   if ((gCurrentSmiHandler != NULL) && (gCurrentSmiHandler != SmiHandler)) {
>     return EFI_INVALID_PARAMETER;
>   }
> 
> ?
> 
> (2) Why do we call the new global variable "gCurrentSmiHandler" rather than
> "mCurrentSmiHandler"?
> 
> Thanks
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116108): https://edk2.groups.io/g/devel/message/116108
Mute This Topic: https://groups.io/mt/104616993/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/4] StandaloneMmPkg: Support to unregister MMI handler inside MMI handler

[Zhiguang Liu]

To support unregister MMI handler inside MMI handler itself,
get next node before MMI handler is executed, since LIST_ENTRY that
Link points to may be freed if unregister MMI handler in MMI handler
itself.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..c1a1d76e85 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -154,9 +154,14 @@ MmiManage (
     Head = &MmiEntry->MmiHandlers;
   }
 
-  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
+  for (Link = Head->ForwardLink; Link != Head;) {
     MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
-
+    //
+    // To support unregister MMI handler inside MMI handler itself,
+    // get next node before handler is executed, since LIST_ENTRY that
+    // Link points to may be freed if unregister MMI handler.
+    //
+    Link   = Link->ForwardLink;
     Status = MmiHandler->Handler (
                            (EFI_HANDLE)MmiHandler,
                            Context,
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116074): https://edk2.groups.io/g/devel/message/116074
Mute This Topic: https://groups.io/mt/104616994/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/4] StandaloneMmPkg: Support to unregister MMI handler inside MMI handler

[Laszlo Ersek]

On 2/28/24 03:27, Zhiguang Liu wrote:
> To support unregister MMI handler inside MMI handler itself,
> get next node before MMI handler is executed, since LIST_ENTRY that
> Link points to may be freed if unregister MMI handler in MMI handler
> itself.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  StandaloneMmPkg/Core/Mmi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
> index 0de6fd17fc..c1a1d76e85 100644
> --- a/StandaloneMmPkg/Core/Mmi.c
> +++ b/StandaloneMmPkg/Core/Mmi.c
> @@ -154,9 +154,14 @@ MmiManage (
>      Head = &MmiEntry->MmiHandlers;
>    }
>  
> -  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
> +  for (Link = Head->ForwardLink; Link != Head;) {
>      MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
> -
> +    //
> +    // To support unregister MMI handler inside MMI handler itself,
> +    // get next node before handler is executed, since LIST_ENTRY that
> +    // Link points to may be freed if unregister MMI handler.
> +    //
> +    Link   = Link->ForwardLink;
>      Status = MmiHandler->Handler (
>                             (EFI_HANDLE)MmiHandler,
>                             Context,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116102): https://edk2.groups.io/g/devel/message/116102
Mute This Topic: https://groups.io/mt/104616994/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/4] StandaloneMmPkg: Support to unregister MMI handler inside MMI handler

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 28, 2024 4:47 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.com>
> Subject: Re: [edk2-devel] [PATCH v2 3/4] StandaloneMmPkg: Support to
> unregister MMI handler inside MMI handler
> 
> On 2/28/24 03:27, Zhiguang Liu wrote:
> > To support unregister MMI handler inside MMI handler itself,
> > get next node before MMI handler is executed, since LIST_ENTRY that
> > Link points to may be freed if unregister MMI handler in MMI handler
> > itself.
> >
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >  StandaloneMmPkg/Core/Mmi.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/StandaloneMmPkg/Core/Mmi.c
> b/StandaloneMmPkg/Core/Mmi.c
> > index 0de6fd17fc..c1a1d76e85 100644
> > --- a/StandaloneMmPkg/Core/Mmi.c
> > +++ b/StandaloneMmPkg/Core/Mmi.c
> > @@ -154,9 +154,14 @@ MmiManage (
> >      Head = &MmiEntry->MmiHandlers;
> >    }
> >
> > -  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
> > +  for (Link = Head->ForwardLink; Link != Head;) {
> >      MmiHandler = CR (Link, MMI_HANDLER, Link,
> MMI_HANDLER_SIGNATURE);
> > -
> > +    //
> > +    // To support unregister MMI handler inside MMI handler itself,
> > +    // get next node before handler is executed, since LIST_ENTRY that
> > +    // Link points to may be freed if unregister MMI handler.
> > +    //
> > +    Link   = Link->ForwardLink;
> >      Status = MmiHandler->Handler (
> >                             (EFI_HANDLE)MmiHandler,
> >                             Context,
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116163): https://edk2.groups.io/g/devel/message/116163
Mute This Topic: https://groups.io/mt/104616994/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/4] StandaloneMmPkg: Support to unregister MMI handler inside MMI handler

[Ard Biesheuvel]

On Wed, 28 Feb 2024 at 03:28, Zhiguang Liu <zhiguang.liu@intel.com> wrote:
>
> To support unregister MMI handler inside MMI handler itself,
> get next node before MMI handler is executed, since LIST_ENTRY that
> Link points to may be freed if unregister MMI handler in MMI handler
> itself.
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  StandaloneMmPkg/Core/Mmi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
> index 0de6fd17fc..c1a1d76e85 100644
> --- a/StandaloneMmPkg/Core/Mmi.c
> +++ b/StandaloneMmPkg/Core/Mmi.c
> @@ -154,9 +154,14 @@ MmiManage (
>      Head = &MmiEntry->MmiHandlers;
>    }
>
> -  for (Link = Head->ForwardLink; Link != Head; Link = Link->ForwardLink) {
> +  for (Link = Head->ForwardLink; Link != Head;) {
>      MmiHandler = CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE);
> -
> +    //
> +    // To support unregister MMI handler inside MMI handler itself,
> +    // get next node before handler is executed, since LIST_ENTRY that
> +    // Link points to may be freed if unregister MMI handler.
> +    //
> +    Link   = Link->ForwardLink;
>      Status = MmiHandler->Handler (
>                             (EFI_HANDLE)MmiHandler,
>                             Context,
> --
> 2.31.1.windows.1
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116629): https://edk2.groups.io/g/devel/message/116629
Mute This Topic: https://groups.io/mt/104616994/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/4] StandaloneMmPkg: Disallow unregister MMI handler in other MMI handler

[Zhiguang Liu]

In last patch, we add code support to unregister MMI handler inside
itself. However, the code doesn't support unregister MMI handler
insider other MMI handler. While this is not a must-have usage.
So add check to disallow unregister MMI handler in other MMI handler.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index c1a1d76e85..54794c6b3d 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -36,8 +36,9 @@ typedef struct {
   MMI_ENTRY                     *MmiEntry;
 } MMI_HANDLER;
 
-LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
-LIST_ENTRY  mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
+LIST_ENTRY   mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
+LIST_ENTRY   mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
+MMI_HANDLER  *gCurrentMmiHandler = NULL;
 
 /**
   Finds the MMI entry for the requested handler type.
@@ -161,13 +162,19 @@ MmiManage (
     // get next node before handler is executed, since LIST_ENTRY that
     // Link points to may be freed if unregister MMI handler.
     //
-    Link   = Link->ForwardLink;
-    Status = MmiHandler->Handler (
-                           (EFI_HANDLE)MmiHandler,
-                           Context,
-                           CommBuffer,
-                           CommBufferSize
-                           );
+    Link = Link->ForwardLink;
+    //
+    // Assign gCurrentMmiHandle before calling the MMI handler and
+    // set to NULL when it returns.
+    //
+    gCurrentMmiHandler = MmiHandler;
+    Status             = MmiHandler->Handler (
+                                       (EFI_HANDLE)MmiHandler,
+                                       Context,
+                                       CommBuffer,
+                                       CommBufferSize
+                                       );
+    gCurrentMmiHandler = NULL;
 
     switch (Status) {
       case EFI_INTERRUPT_PENDING:
@@ -314,6 +321,16 @@ MmiHandlerUnRegister (
     return EFI_INVALID_PARAMETER;
   }
 
+  //
+  // Check if unregister MMI handler inside a MMI Handler
+  //
+  if (gCurrentMmiHandler != NULL) {
+    //
+    // Only allow to unregister MMI Handler inside itself.
+    //
+    ASSERT (gCurrentMmiHandler == MmiHandler);
+  }
+
   MmiEntry = MmiHandler->MmiEntry;
 
   RemoveEntryList (&MmiHandler->Link);
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116075): https://edk2.groups.io/g/devel/message/116075
Mute This Topic: https://groups.io/mt/104616995/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/4] StandaloneMmPkg: Disallow unregister MMI handler in other MMI handler

[Laszlo Ersek]

On 2/28/24 03:27, Zhiguang Liu wrote:
> In last patch, we add code support to unregister MMI handler inside
> itself. However, the code doesn't support unregister MMI handler
> insider other MMI handler. While this is not a must-have usage.
> So add check to disallow unregister MMI handler in other MMI handler.
> 
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  StandaloneMmPkg/Core/Mmi.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
> index c1a1d76e85..54794c6b3d 100644
> --- a/StandaloneMmPkg/Core/Mmi.c
> +++ b/StandaloneMmPkg/Core/Mmi.c
> @@ -36,8 +36,9 @@ typedef struct {
>    MMI_ENTRY                     *MmiEntry;
>  } MMI_HANDLER;
>  
> -LIST_ENTRY  mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
> -LIST_ENTRY  mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
> +LIST_ENTRY   mRootMmiHandlerList = INITIALIZE_LIST_HEAD_VARIABLE (mRootMmiHandlerList);
> +LIST_ENTRY   mMmiEntryList       = INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
> +MMI_HANDLER  *gCurrentMmiHandler = NULL;
>  
>  /**
>    Finds the MMI entry for the requested handler type.
> @@ -161,13 +162,19 @@ MmiManage (
>      // get next node before handler is executed, since LIST_ENTRY that
>      // Link points to may be freed if unregister MMI handler.
>      //
> -    Link   = Link->ForwardLink;
> -    Status = MmiHandler->Handler (
> -                           (EFI_HANDLE)MmiHandler,
> -                           Context,
> -                           CommBuffer,
> -                           CommBufferSize
> -                           );
> +    Link = Link->ForwardLink;
> +    //
> +    // Assign gCurrentMmiHandle before calling the MMI handler and
> +    // set to NULL when it returns.
> +    //
> +    gCurrentMmiHandler = MmiHandler;
> +    Status             = MmiHandler->Handler (
> +                                       (EFI_HANDLE)MmiHandler,
> +                                       Context,
> +                                       CommBuffer,
> +                                       CommBufferSize
> +                                       );
> +    gCurrentMmiHandler = NULL;
>  
>      switch (Status) {
>        case EFI_INTERRUPT_PENDING:
> @@ -314,6 +321,16 @@ MmiHandlerUnRegister (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> +  //
> +  // Check if unregister MMI handler inside a MMI Handler
> +  //
> +  if (gCurrentMmiHandler != NULL) {
> +    //
> +    // Only allow to unregister MMI Handler inside itself.
> +    //
> +    ASSERT (gCurrentMmiHandler == MmiHandler);
> +  }
> +
>    MmiEntry = MmiHandler->MmiEntry;
>  
>    RemoveEntryList (&MmiHandler->Link);

same comment as for patch#2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116103): https://edk2.groups.io/g/devel/message/116103
Mute This Topic: https://groups.io/mt/104616995/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-