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

["Laszlo Ersek" <lersek@redhat.com>]

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 (#114384): https://edk2.groups.io/g/devel/message/114384
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]
-=-=-=-=-=-=-=-=-=-=-=-