From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7BCDE740046 for ; Thu, 7 Mar 2024 09:06:52 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=g1kzliEnM0swb/o1igm8eezVcjL7vmgnGW8lmAlFM4Y=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20240206; t=1709802411; v=1; b=p8hjcF3ZWJyKjxmrz0dQjR9+mgogrj0EjoGWHVe1nHQ71eJxXMhwFD/UULM/703I/++TRZFq muukTeWhAaEX64lpiIH8M0BkwiQOht2EOcHTifz96h6+7x0PQuVTIKAum20gk8vc6rsiL2Vbl86 dtQPRwvIGeUAftaN963QDpKPjBQ1E4+l3UQQ50bC76g3Zje72+kAKE3BcPVnXoQb3NWFxlS3R5C PaWUKBmIlxRwrps+613ZRfppPT6KuTBwLIlRA26ZmMjx5CAiCdpEPQ7W8Opd7tEB1O/vM9Z1XMd 8+4WBrHBWdd0SjAJl21U08jRihm8Nn0BrlcS5n1aJbyNA== X-Received: by 127.0.0.2 with SMTP id u90UYY7687511xxh4xNpioSz; Thu, 07 Mar 2024 01:06:51 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.19154.1709802410549210103 for ; Thu, 07 Mar 2024 01:06:50 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-590-FGZzP4e_Nt-8j_JVM5evxA-1; Thu, 07 Mar 2024 04:06:44 -0500 X-MC-Unique: FGZzP4e_Nt-8j_JVM5evxA-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E6264857F51; Thu, 7 Mar 2024 09:06:43 +0000 (UTC) X-Received: from [10.39.193.113] (unknown [10.39.193.113]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 39955C01602; Thu, 7 Mar 2024 09:06:42 +0000 (UTC) Message-ID: <92e0f955-2b0c-9b80-9ef3-7ae711899945@redhat.com> Date: Thu, 07 Mar 2024 01:06:50 -0800 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler To: "Liu, Zhiguang" , "devel@edk2.groups.io" Cc: Liming Gao , "Wu, Jiaxin" , "Ni, Ray" , Ard Biesheuvel , Sami Mujawar References: <20240124040301.2176-1-zhiguang.liu@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: VtaksGtNTr7AatVbvRdjBW9qx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=p8hjcF3Z; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 3/7/24 07:11, Liu, Zhiguang wrote: > Hi Laszlo, >=20 > We talked about how to unregister SMI handler inside other SMI handler. > And the conclusion was there is no such usage so we don't need support it= . > However, I find some platform does need to unregister SMI handler inside = other SMI handler. :) > The design you introduced below is great to meet the needs. I'm happy to hear that! > If you don't have time to implement and have no concern, I can implement = your design and send the patch out. Yes, please go ahead and implement it, if you think it fits your needs. I'll probably not be around to review it, though. Cheers! Laszlo >=20 > Thanks > Zhiguang >=20 >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Thursday, January 25, 2024 6:48 PM >> To: devel@edk2.groups.io; Liu, Zhiguang >> Cc: Liming Gao ; Wu, Jiaxin >> ; Ni, Ray ; Ard Biesheuvel >> ; Sami Mujawar >> 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 >>> Cc: Jiaxin Wu >>> Cc: Ray Ni >>> Signed-off-by: Zhiguang Liu >>> --- >>> 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 =3D &SmiEntry->SmiHandlers; >>> >>> - for (Link =3D Head->ForwardLink; Link !=3D Head; Link =3D >>> Link->ForwardLink) { >>> + for (Link =3D Head->ForwardLink; Link !=3D Head;) { >>> SmiHandler =3D 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 =3D Link->ForwardLink; >>> >>> Status =3D 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 t= he >> 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 th= at 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=3D0, Managing=3D1, CleanupNeeded= =3D2). >> >> - a new "BOOLEAN Deleted" field in SMI_HANDLER >> >> - all loops in "Smi.c" iterating over SMI_HANDLERs would have to skip ha= ndlers >> that have been marked as Deleted >> >> - in SmiManage(), set the state to Managing (=3D1) before the loop. Afte= r the >> loop, check if the state is CleanupNeeded (=3D2); if so, add an extra pa= ss for >> actually deleting SMI_HANDLERs from the list that have been marked Delet= ed. >> Finally (regardless of the state being Managing (=3D1) or CleanupNeeded = (=3D2)), >> reset the state to NotManaging (=3D0). >> >> - in SmiHandlerUnRegister(), if the state is NotManaging (=3D0), delete = the >> handler immediately. Otherwise (i.e., when the state is Managing >> (=3D1) or CleanupNeeded (=3D2)), set the state to CleanupNeeded (=3D2), = 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 t= o 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 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116479): https://edk2.groups.io/g/devel/message/116479 Mute This Topic: https://groups.io/mt/103925794/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-