* [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler @ 2024-01-24 4:03 Zhiguang Liu 2024-01-24 8:11 ` Ni, Ray 2024-01-25 10:48 ` Laszlo Ersek 0 siblings, 2 replies; 12+ messages in thread From: Zhiguang Liu @ 2024-01-24 4:03 UTC (permalink / raw) To: devel; +Cc: Zhiguang Liu, Liming Gao, Jiaxin Wu, Ray Ni 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, -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114251): https://edk2.groups.io/g/devel/message/114251 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 4:03 [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler Zhiguang Liu @ 2024-01-24 8:11 ` Ni, Ray 2024-01-24 13:06 ` Laszlo Ersek 2024-01-25 10:48 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Ni, Ray @ 2024-01-24 8:11 UTC (permalink / raw) To: Liu, Zhiguang, devel@edk2.groups.io Cc: Liming Gao, Wu, Jiaxin, POLUDOV, FELIX Felix, I remember you mentioned to me about the usage of SMI handler unregistering itself. Reviewed-by: Ray Ni <ray.ni@intel.com> Thanks, Ray > -----Original Message----- > From: Liu, Zhiguang <zhiguang.liu@intel.com> > Sent: Wednesday, January 24, 2024 12:03 PM > 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> > Subject: [PATCH] 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> > 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, > -- > 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114269): https://edk2.groups.io/g/devel/message/114269 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 8:11 ` Ni, Ray @ 2024-01-24 13:06 ` Laszlo Ersek 2024-01-24 16:17 ` Ni, Ray 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2024-01-24 13:06 UTC (permalink / raw) To: devel, ray.ni, Liu, Zhiguang; +Cc: Liming Gao, Wu, Jiaxin, POLUDOV, FELIX On 1/24/24 09:11, Ni, Ray wrote: > Felix, I remember you mentioned to me about the usage of SMI handler unregistering itself. I wanted to ask: is this something that the PI spec comments on? I.e., is this usage expected by the spec (in which case this bugfix is a conformance fix), or is the spec silent on it (in which case I guess we can call this a quality-of-implementation improvement)? > > Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > > Thanks, > Ray >> -----Original Message----- >> From: Liu, Zhiguang <zhiguang.liu@intel.com> >> Sent: Wednesday, January 24, 2024 12:03 PM >> 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> >> Subject: [PATCH] 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> >> 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, >> -- >> 2.31.1.windows.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114291): https://edk2.groups.io/g/devel/message/114291 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 13:06 ` Laszlo Ersek @ 2024-01-24 16:17 ` Ni, Ray 2024-01-24 17:15 ` Felix Polyudov via groups.io 0 siblings, 1 reply; 12+ messages in thread From: Ni, Ray @ 2024-01-24 16:17 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Liu, Zhiguang Cc: Liming Gao, Wu, Jiaxin, POLUDOV, FELIX [-- Attachment #1: Type: text/plain, Size: 3389 bytes --] spec does not say the unregistration is allowed inside handler. it's just to improve the qualiquali the code. thanks, ray ________________________________ From: Laszlo Ersek <lersek@redhat.com> Sent: Wednesday, January 24, 2024 9:06:13 PM To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; POLUDOV, FELIX <felixp@ami.com> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler On 1/24/24 09:11, Ni, Ray wrote: > Felix, I remember you mentioned to me about the usage of SMI handler unregistering itself. I wanted to ask: is this something that the PI spec comments on? I.e., is this usage expected by the spec (in which case this bugfix is a conformance fix), or is the spec silent on it (in which case I guess we can call this a quality-of-implementation improvement)? > > Reviewed-by: Ray Ni <ray.ni@intel.com> Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > > Thanks, > Ray >> -----Original Message----- >> From: Liu, Zhiguang <zhiguang.liu@intel.com> >> Sent: Wednesday, January 24, 2024 12:03 PM >> 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> >> Subject: [PATCH] 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> >> 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, >> -- >> 2.31.1.windows.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114317): https://edk2.groups.io/g/devel/message/114317 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] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 5497 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 16:17 ` Ni, Ray @ 2024-01-24 17:15 ` Felix Polyudov via groups.io 2024-01-24 17:19 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Felix Polyudov via groups.io @ 2024-01-24 17:15 UTC (permalink / raw) To: Ni, Ray, Laszlo Ersek, devel@edk2.groups.io, Liu, Zhiguang Cc: Liming Gao, Wu, Jiaxin [-- Attachment #1: Type: text/plain, Size: 5598 bytes --] In my opinion this is not a spec conformance fix, but it is a bug fix. The spec does not explicitly mentions unregistering from within the handler, but by applying a principle that everything that's not forbidden is legal, it's fair for a caller to expect that unregistering works fine in any valid context including from within the MM handler. The spec does mention that UEFI/PI interfaces are not reentrant, however, reentrancy typically implies repetitive invocation of the same interface, not a call to one MM Core interface while another MM Core interface is in progress. From: Ni, Ray <ray.ni@intel.com> Sent: Wednesday, January 24, 2024 11:17 AM To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Felix Polyudov <Felixp@ami.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler **CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.** spec does not say the unregistration is allowed inside handler. it's just to improve the qualiquali the code. thanks, ray ________________________________ From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Sent: Wednesday, January 24, 2024 9:06:13 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>> Cc: Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Wu, Jiaxin <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>>; POLUDOV, FELIX <felixp@ami.com<mailto:felixp@ami.com>> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler On 1/24/24 09:11, Ni, Ray wrote: > Felix, I remember you mentioned to me about the usage of SMI handler unregistering itself. I wanted to ask: is this something that the PI spec comments on? I.e., is this usage expected by the spec (in which case this bugfix is a conformance fix), or is the spec silent on it (in which case I guess we can call this a quality-of-implementation improvement)? > > Reviewed-by: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> Reviewed-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Thanks Laszlo > > Thanks, > Ray >> -----Original Message----- >> From: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>> >> Sent: Wednesday, January 24, 2024 12:03 PM >> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >> Cc: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Liming Gao >> <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Wu, Jiaxin <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>>; Ni, Ray >> <ray.ni@intel.com<mailto:ray.ni@intel.com>> >> Subject: [PATCH] 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<mailto:gaoliming@byosoft.com.cn>> >> Cc: Jiaxin Wu <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>> >> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> >> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com<mailto: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, >> -- >> 2.31.1.windows.1 > > > > > > -The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114340): https://edk2.groups.io/g/devel/message/114340 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] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 11727 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 17:15 ` Felix Polyudov via groups.io @ 2024-01-24 17:19 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2024-01-24 17:19 UTC (permalink / raw) To: devel, felixp, Ni, Ray, Liu, Zhiguang; +Cc: Liming Gao, Wu, Jiaxin On 1/24/24 18:15, Felix Polyudov via groups.io wrote: > In my opinion this is not a spec conformance fix, but it is a bug fix. > > The spec does not explicitly mentions unregistering from within the handler, > > but by applying a principle that everything that’s not forbidden is legal, > > it’s fair for a caller to expect that unregistering works fine in any > valid context > > including from within the MM handler. > > > > The spec does mention that UEFI/PI interfaces are not reentrant, > > however, reentrancy typically implies repetitive invocation of the same > interface, > > not a call to one MM Core interface while another MM Core interface is > in progress. Sure, I'm not contesting any of that; it's great that you and Ray seem to agree. I just wanted to understand. My R-b stands. Thanks! Laszlo > *From:*Ni, Ray <ray.ni@intel.com> > *Sent:* Wednesday, January 24, 2024 11:17 AM > *To:* Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Liu, > Zhiguang <zhiguang.liu@intel.com> > *Cc:* Liming Gao <gaoliming@byosoft.com.cn>; Wu, Jiaxin > <jiaxin.wu@intel.com>; Felix Polyudov <Felixp@ami.com> > *Subject:* [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support > to unregister SMI handler inside SMI handler > > > > > > ***CAUTION:*The e-mail below is from an external source. Please exercise > caution before opening attachments, clicking links, or following > guidance.** > > spec does not say the unregistration is allowed inside handler. it's > just to improve the qualiquali the code. > > > > thanks, > > ray > > ------------------------------------------------------------------------ > > *From:*Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> > *Sent:* Wednesday, January 24, 2024 9:06:13 PM > *To:* devel@edk2.groups.io <mailto:devel@edk2.groups.io> > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>>; Ni, Ray > <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Liu, Zhiguang > <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>> > *Cc:* Liming Gao <gaoliming@byosoft.com.cn > <mailto:gaoliming@byosoft.com.cn>>; Wu, Jiaxin <jiaxin.wu@intel.com > <mailto:jiaxin.wu@intel.com>>; POLUDOV, FELIX <felixp@ami.com > <mailto:felixp@ami.com>> > *Subject:* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to > unregister SMI handler inside SMI handler > > > > On 1/24/24 09:11, Ni, Ray wrote: >> Felix, I remember you mentioned to me about the usage of SMI handler unregistering itself. > > I wanted to ask: is this something that the PI spec comments on? I.e., > is this usage expected by the spec (in which case this bugfix is a > conformance fix), or is the spec silent on it (in which case I guess we > can call this a quality-of-implementation improvement)? > >> >> Reviewed-by: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>> > > Thanks > Laszlo > >> >> Thanks, >> Ray >>> -----Original Message----- >>> From: Liu, Zhiguang <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>> >>> Sent: Wednesday, January 24, 2024 12:03 PM >>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io> >>> Cc: Liu, Zhiguang <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Liming Gao >>> <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>; Wu, Jiaxin > <jiaxin.wu@intel.com <mailto:jiaxin.wu@intel.com>>; Ni, Ray >>> <ray.ni@intel.com <mailto:ray.ni@intel.com>> >>> Subject: [PATCH] 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 <mailto:gaoliming@byosoft.com.cn>> >>> Cc: Jiaxin Wu <jiaxin.wu@intel.com <mailto:jiaxin.wu@intel.com>> >>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>> >>> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com <mailto: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, >>> -- >>> 2.31.1.windows.1 >> >> >> >> >> >> > > -The information contained in this message may be confidential and > proprietary to American Megatrends (AMI). This communication is intended > to be read only by the individual or entity to whom it is addressed or > by their designee. If the reader of this message is not the intended > recipient, you are on notice that any distribution of this message, in > any form, is strictly prohibited. Please promptly notify the sender by > reply e-mail or by telephone at 770-246-8600, and then delete or destroy > all copies of the transmission. > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114342): https://edk2.groups.io/g/devel/message/114342 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-24 4:03 [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler Zhiguang Liu 2024-01-24 8:11 ` Ni, Ray @ 2024-01-25 10:48 ` Laszlo Ersek 2024-01-25 12:05 ` Ni, Ray 2024-03-07 6:11 ` Zhiguang Liu 1 sibling, 2 replies; 12+ messages in thread From: Laszlo Ersek @ 2024-01-25 10:48 UTC (permalink / raw) To: devel, zhiguang.liu Cc: Liming Gao, Jiaxin Wu, Ray Ni, Ard Biesheuvel, Sami Mujawar 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-25 10:48 ` Laszlo Ersek @ 2024-01-25 12:05 ` Ni, Ray 2024-01-25 19:25 ` Laszlo Ersek 2024-03-07 6:11 ` Zhiguang Liu 1 sibling, 1 reply; 12+ messages in thread From: Ni, Ray @ 2024-01-25 12:05 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Liu, Zhiguang Cc: Liming Gao, Wu, Jiaxin, Ard Biesheuvel, Sami Mujawar 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-25 12:05 ` Ni, Ray @ 2024-01-25 19:25 ` Laszlo Ersek 2024-01-26 2:17 ` Ni, Ray 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2024-01-25 19:25 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io, Liu, Zhiguang Cc: Liming Gao, Wu, Jiaxin, Ard Biesheuvel, Sami Mujawar On 1/25/24 13:05, Ni, Ray wrote: > 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. Sounds safe enough. I don't know if it conforms to the spec however (although we might just choose not to care about that). Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114458): https://edk2.groups.io/g/devel/message/114458 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-25 19:25 ` Laszlo Ersek @ 2024-01-26 2:17 ` Ni, Ray 0 siblings, 0 replies; 12+ messages in thread From: Ni, Ray @ 2024-01-26 2:17 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Liu, Zhiguang Cc: Liming Gao, Wu, Jiaxin, Ard Biesheuvel, Sami Mujawar > Sounds safe enough. I don't know if it conforms to the spec however > (although we might just choose not to care about that). Should be compliant to spec. Or I should say spec does not under which case the Unregister() should succeed. As long as spec allows Unregister() returns a failure status, a very low-quality MmCore could always return failure from Unregister(). Our choice here is to return failure for a special case only (Unregister handler B in handler A). > > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114483): https://edk2.groups.io/g/devel/message/114483 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-01-25 10:48 ` Laszlo Ersek 2024-01-25 12:05 ` Ni, Ray @ 2024-03-07 6:11 ` Zhiguang Liu 2024-03-07 9:06 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Zhiguang Liu @ 2024-03-07 6:11 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io Cc: Liming Gao, Wu, Jiaxin, Ni, Ray, Ard Biesheuvel, Sami Mujawar Hi Laszlo, 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. If you don't have time to implement and have no concern, I can implement your design and send the patch out. Thanks Zhiguang > -----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 (#116476): https://edk2.groups.io/g/devel/message/116476 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler 2024-03-07 6:11 ` Zhiguang Liu @ 2024-03-07 9:06 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2024-03-07 9:06 UTC (permalink / raw) To: Liu, Zhiguang, devel@edk2.groups.io Cc: Liming Gao, Wu, Jiaxin, Ni, Ray, Ard Biesheuvel, Sami Mujawar On 3/7/24 07:11, Liu, Zhiguang wrote: > Hi Laszlo, > > 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 > > Thanks > Zhiguang > >> -----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 (#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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-03-07 9:06 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-24 4:03 [edk2-devel] [PATCH] MdeModulePkg/SMM: Support to unregister SMI handler inside SMI handler Zhiguang Liu 2024-01-24 8:11 ` Ni, Ray 2024-01-24 13:06 ` Laszlo Ersek 2024-01-24 16:17 ` Ni, Ray 2024-01-24 17:15 ` Felix Polyudov via groups.io 2024-01-24 17:19 ` Laszlo Ersek 2024-01-25 10:48 ` Laszlo Ersek 2024-01-25 12:05 ` Ni, Ray 2024-01-25 19:25 ` Laszlo Ersek 2024-01-26 2:17 ` Ni, Ray 2024-03-07 6:11 ` Zhiguang Liu 2024-03-07 9:06 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox