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

["Ni, Ray" <ray.ni@intel.com>]

Laszlo,
SMI handler is called from SmmCore.
So SmmCore knows which handle is passed to the SMI handler.
How about let Unregister() rejects the request coming from a SMI handler which unregisters other handles?

A "gCurrentSmiHandle" can be assigned before calling the SMI handler and set to NULL when it returns.
Unregister() compares the handle against gCurrentSmiHandle if it's not NULL.

Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 25, 2024 6:48 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] MdeModulePkg/SMM: Support to
> unregister SMI handler inside SMI handler
> 
> On 1/24/24 05:03, Zhiguang Liu wrote:
> > 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>
> > Cc: Ray Ni <ray.ni@intel.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..a75e52b1ae 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 unregiser 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,
> 
> I've had a further thought here.
> 
> (1) This patch may enable an SMI handler to unregister *itself*, that
> is, through the following call chain
> 
>   SmiManage()
>     SmiHandler->Handler()
>       SmiHandlerUnRegister()
> 
> but it still does not ensure *general iterator safety*.
> 
> Assume that an SMM driver registers two handlers, handler A and handler
> B. Assume the DispatchHandles for these are stored in global variables
> in the driver. Then, assume that "handler A" (for whatever reason, under
> some circumstances) unregisters "handler B".
> 
> That could still lead to a use-after-free in the SmiManage() loop; is
> that right?
> 
> If that driver scenario is plausible, then something like the following
> would be needed:
> 
> - a global variable of enum type in "MdeModulePkg/Core/PiSmmCore/Smi.c",
> with three possible values (NotManaging=0, Managing=1, CleanupNeeded=2).
> 
> - a new "BOOLEAN Deleted" field in SMI_HANDLER
> 
> - all loops in "Smi.c" iterating over SMI_HANDLERs would have to skip
> handlers that have been marked as Deleted
> 
> - in SmiManage(), set the state to Managing (=1) before the loop. After
> the loop, check if the state is CleanupNeeded (=2); if so, add an extra
> pass for actually deleting SMI_HANDLERs from the list that have been
> marked Deleted. Finally (regardless of the state being Managing (=1) or
> CleanupNeeded (=2)), reset the state to NotManaging (=0).
> 
> - in SmiHandlerUnRegister(), if the state is NotManaging (=0), delete
> the handler immediately. Otherwise (i.e., when the state is Managing
> (=1) or CleanupNeeded (=2)), set the state to CleanupNeeded (=2), and
> only mark the handler as Deleted.
> 
> The idea is to inform SmiHandlerUnRegister() whether it's running or not
> running on the stack of SmiManage(), and to postpone SMI_HANDLER
> deletion until after the loop finishes in the former case.
> 
> I'm not sure if real life SmiHandlerUnRegister() usage is worth this
> complication, however.
> 
> (2) Independently: does the same issue exist for the StMM core? I seem
> to be discovering the same problem in MmiManage()
> [StandaloneMmPkg/Core/Mmi.c].
> 
> So, whatever patch we add to the SMM_CORE, should likely be reflected to
> MM_CORE_STANDALONE too. Adding Ard and Sami to the CC list.
> 
> Thanks
> Laszlo



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