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 <ler...@redhat.com>
> Sent: Thursday, January 25, 2024 6:48 PM
> To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang....@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>; Wu, Jiaxin
> <jiaxin...@intel.com>; Ni, Ray <ray...@intel.com>; Ard Biesheuvel
> <ardb+tianoc...@kernel.org>; Sami Mujawar <sami.muja...@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 <gaolim...@byosoft.com.cn>
> > Cc: Jiaxin Wu <jiaxin...@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Signed-off-by: Zhiguang Liu <zhiguang....@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
