From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail04.groups.io (mail04.groups.io [45.79.224.9]) by spool.mail.gandi.net (Postfix) with ESMTPS id 34B1D941043 for ; Tue, 16 Apr 2024 03:52:50 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=p94biQj9A4c4RQJLRKa0oPVYuIJnw6L6Llhh3ICHPHc=; c=relaxed/simple; d=groups.io; h=From:To:CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:msip_labels:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type; s=20240206; t=1713239569; v=1; b=hTclFAYQdk7NDpZ87n5gxMhZZyQPIfEQ6Ds5oV4Er25EllV4zqwRBTXVf8OiqVk9P9kc5k5/ CTVuVavGpMNI/slME+85F+KnLBaUQdZfYAv75N4aN0O5jzCbXAfe6FAPbUSUZCLliLN1IN6fFPY lQcMJBnrSNEGqgbM6/4raSPKJBhWkA619EYKCRWKu0ANczCPT6oSGxm5Z1GITgP44/dUFQBcgGV 5n1Qpd1b6deq1dXiGUssw0akm4xQmVf//bNhL6OtdkYq7zXCjp5T5gqTC4o9AWUQHjJgGq4P6VC PsFqGL+NPQ7ffT1eN/v3WvunTBaLgIYJMKHDEK6r8Ggkg== X-Received: by 127.0.0.2 with SMTP id Y8sIYY7687511xbtCWxbViPu; Mon, 15 Apr 2024 20:52:49 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.14]) by mx.groups.io with SMTP id smtpd.web11.11840.1713239569142165527 for ; Mon, 15 Apr 2024 20:52:49 -0700 X-CSE-ConnectionGUID: kVj+xYphQSeScFuTxpq21Q== X-CSE-MsgGUID: p50B6b+WT22rpCrZq1YUSw== X-IronPort-AV: E=McAfee;i="6600,9927,11045"; a="8875969" X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="8875969" X-Received: from orviesa009.jf.intel.com ([10.64.159.149]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2024 20:52:49 -0700 X-CSE-ConnectionGUID: q/1XMu6hQCanfYfKOna+0g== X-CSE-MsgGUID: DlnRQfA+QpWSmCRO5ZEWdA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="22121690" X-Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orviesa009.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 15 Apr 2024 20:52:48 -0700 X-Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Mon, 15 Apr 2024 20:52:47 -0700 X-Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend Transport; Mon, 15 Apr 2024 20:52:47 -0700 X-Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.168) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Mon, 15 Apr 2024 20:52:47 -0700 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by MN2PR11MB4712.namprd11.prod.outlook.com (2603:10b6:208:264::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7472.30; Tue, 16 Apr 2024 03:52:45 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::8774:81a7:c5b7:5c2c]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::8774:81a7:c5b7:5c2c%7]) with mapi id 15.20.7452.049; Tue, 16 Apr 2024 03:52:45 +0000 From: "Ni, Ray" To: "Liu, Zhiguang" , "devel@edk2.groups.io" CC: Liming Gao , "Wu, Jiaxin" , Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Thread-Topic: [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler in SMI handlers Thread-Index: AQHaj6ebnrgBFjvh3EyzyM4yMbda27FqQ7cs Date: Tue, 16 Apr 2024 03:52:45 +0000 Message-ID: References: <20240416024108.1358-1-zhiguang.liu@intel.com> <20240416024108.1358-6-zhiguang.liu@intel.com> In-Reply-To: <20240416024108.1358-6-zhiguang.liu@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|MN2PR11MB4712:EE_ x-ms-office365-filtering-correlation-id: 5166fd08-f725-4212-e471-08dc5dc8ad1d x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: tcXHnK3GuaZDxK2bLfN7tshS3Mc/2m/35B35IMlWSUMSabPbhqNPoLCCnRzJdGRbB9zQd4GBZRz8O7mmyvdmUQuz6WjmvvKnRdC3vYAU18T/LsKPPeHJcYLWa5Y7q4+foJ45rRpOF78BFWnPIyqYrygSOBho8523d1nnijWs/vXRPfNgAsCOFUAosTJh0ulR7JxWej9TlVfnyaADMBpfNmvzF/l9u+mPOjWKXrlkLC0ZAvYvIo3N7QwZq8ZMwonNJnRpo0EL+cbAgBhKkFN7kV8fnM50VyMaXKozNNNIxxE7CNy35csc7BLzrWdbGg/xijB6+zee0lnVuwSY1iRSHL4/bAwMyNU0XZI9xzjPXIZ8dkQtZpNfJpO15XGvI5Ke/OCrYPydo6KXYi5vJknFZ22mjE2WfTb83OqqVTdU7nQfMjDF65pefn8g6URQ1Nz1rhgGw0r7OLt9UrnJrs1F4esjCKX775GLMJcXcKmevX8VRd0kF+xZAuXmbuDl49yzZGc4uy41TQHfzVSTR2K/KS6R8NFnfh3OQ7DYLAzInEGcjY++3sS2Sc1WT90Puh+HwoUMCAe6Mrt86Ir+X9Wlz/x8mGAW8Emp9n5u1xPQdsCVlO1u/46c8d/fm5JVz2Q3662IzpJ6bhrdgpi+K7j5dz56V/PCxOORMoYCJk8jEJSOWk0UMh9LIfcNcLNqQDbHD+WkWaJnhErYLNaIKe8bWtC7EM00ildgW47QPnae8wE= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?axS8WZ3x5UFWM5oAbgN/Id8ae70QDfqfEB4py21+5MTgBvkWDfHOXfjY/tKc?= =?us-ascii?Q?VTreyBoj74u0nelBlvIJ1iQpkytz5qZj3FeGk9K7lPJYkj5rPkNQaTc+nCra?= =?us-ascii?Q?A86dILXBA0ayC/40+3qonjzpjiMoMGAPEPV0mPGZ48yGBxJD5zGF3cM3ipxO?= =?us-ascii?Q?lyrYvGOJRpBN7YIVqtAxnqwM4E71kjrK14S3Y0bprSFX+/DcN5pgDBXazfu/?= =?us-ascii?Q?d92ohupgVH5ZtZIKxFXDEl+ydIpTE+5xKxS07w44AVKo/NY7qKAaQzfTwc/S?= =?us-ascii?Q?dR1VQ02Z35CR+tOHCraFRNCgeIfPrdRgczvrgQyD4/yZFS3N1nC4K75IA+h2?= =?us-ascii?Q?brP8nL3FnPEiMHM2An1djw3DDonTaufzTihsH5liKi1RPb8L2EcurNSOd0Hl?= =?us-ascii?Q?baX18wx9NjYqKy10rNqhXjEnWJZoD4PVgrCTPGL471w39fymzKrduTlB0uA4?= =?us-ascii?Q?wedGGVBMlnzBrF8gZ0LtxBp+gUPIv+SmKi8vIkoQ4ufmHt1ixXAzue+O2/fC?= =?us-ascii?Q?S1ftlFq6xothn89yAudmhuwmh/tCyKnHYv2GvgaT3wfpC1kiR9Dlw8YVfVgB?= =?us-ascii?Q?btZ1N0rxmQXALTu7phg7V89CHCz/yRafehu5ac7UGZv15JOeO0xEun+994MU?= =?us-ascii?Q?Dhymg0o7dkSaigHS2sc1LvvQ6txSGTSqrQeDNFtXz7S83SxXr0zzvM1E6YL1?= =?us-ascii?Q?08wz2hxGR5vV0WJ8nxlQVHTGaMwpL7+2qyEWLjHzkYbVCyEvUx1N+PGhY7a+?= =?us-ascii?Q?IXt8LybF560j0ZElju1/Ki3MC6F+uWR0ov0JsPWwrYeQTG8LTfnd+fbHeIx/?= =?us-ascii?Q?S8oO32Ck+SI26+9Jcmn7LQNTyUVoOKviRfwvCmDxPomYhoXHVlRodBF0fW8L?= =?us-ascii?Q?r2N79djtoHS6Mc51l6KYSwEC/S87zotw08hkr6JrycHYv82de2/eytr7zUoU?= =?us-ascii?Q?u/nWiDFBzMf6UYPK7zdMSGfhKYR1VnqoG3o4q9HSixRn2uMlfaLQuqDHC5VA?= =?us-ascii?Q?1mPEm8KSaJ3reDK2ZVceqZckdgPgrQfgEp/yHXUTZimBFUvP0yks236uYU8c?= =?us-ascii?Q?JeZ/TQZEEEFmBYobZlB6D6vhcM/cbxXxhop+E/2E0B/tzYdF+70PF5fBi1Gx?= =?us-ascii?Q?4ns6LhYzkNU6xUpVdQUJo7/uZGycT4RVbMxL+DHVpgY6Tjs8/w2eGqeESzvn?= =?us-ascii?Q?dSyBK6IAb3np4XBa2g33FHBBuhQWPJVs+3WhiV3tk1B3k4kkyZP47reY8+Fh?= =?us-ascii?Q?5h+JBpLat/WrpxYcQyFNNNgsorLy9ZE/p0He6QLsmLZSRY3t667unCDVh/TQ?= =?us-ascii?Q?AW5/UVjM4K3PXz/7p4sORGms3NAkPC49Lwbbbu4Bil84vIEndfGFhzsbguBJ?= =?us-ascii?Q?lLh/F3YA3Winnl3v2wjeNqe3Kchtj3FO8lZDSHflVZKILmaxFmZ6oH+gW/RL?= =?us-ascii?Q?7mkDIyF+jhFK9OgXJGPVut4HhGnTmt3uPyEz3SfnKoSi08u1/x6k9dCvYM61?= =?us-ascii?Q?HhSFOYMBVP+uHyzn+UxzVEKBTyc5OSibQa3AdIqnSqf1NIJeYoEFPyGB2SuG?= =?us-ascii?Q?gGB+GAgK121kateHbXI=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8244.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5166fd08-f725-4212-e471-08dc5dc8ad1d X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 03:52:45.8818 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: w/0N0NwrT7cgi6abmr4zLOFuSKHJKGSjy/Tk0nYDMplBYc6xMRg5RbYxsk+cy6stZmqOuWx2+RYFGJpyjc32fQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4712 X-OriginatorOrg: intel.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 Resent-Date: Mon, 15 Apr 2024 20:52:49 -0700 Resent-From: ray.ni@intel.com Reply-To: devel@edk2.groups.io,ray.ni@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: SDoCjkqnp80xFizMPNGmGTRsx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244740C2BDED22FC584CCAD8C082MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=hTclFAYQ; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.9 as permitted sender) smtp.mailfrom=bounce@groups.io --_000_MN6PR11MB8244740C2BDED22FC584CCAD8C082MN6PR11MB8244namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni Thanks, Ray ________________________________ From: Liu, Zhiguang Sent: Tuesday, April 16, 2024 10:41 To: devel@edk2.groups.io Cc: Liu, Zhiguang ; Liming Gao ; Wu, Jiaxin ; Ni, Ray ; Laszl= o Ersek Subject: [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI handler= in SMI handlers This patch fix a use-after-free issue where unregistering an SMI handler could lead to the deletion of the SMI_HANDLER while it is still in use by SmiManage(). The fix involves modifying SmiHandlerUnRegister() to detect whether it is being called from within the SmiManage() stack. If so, the removal of the SMI_HANDLER is deferred until SmiManage() has finished executing. Additionally, due to the possibility of recursive SmiManage() calls, the unregistration and subsequent removal of the SMI_HANDLER are ensured to occur only after the outermost SmiManage() invocation has completed. Cc: Liming Gao Cc: Jiaxin Wu Cc: Ray Ni Cc: Laszlo Ersek Signed-off-by: Zhiguang Liu --- MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 1 + MdeModulePkg/Core/PiSmmCore/Smi.c | 164 ++++++++++++++++++++---- 2 files changed, 139 insertions(+), 26 deletions(-) diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/Pi= SmmCore/PiSmmCore.h index b8a490a8c3..60073c78b4 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h @@ -93,6 +93,7 @@ typedef struct { SMI_ENTRY *SmiEntry; VOID *Context; // for profile UINTN ContextSize; // for profile + BOOLEAN ToRemove; // To remove this SMI_HANDL= ER later } SMI_HANDLER; // diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCor= e/Smi.c index 2985f989c3..a84a1f48d3 100644 --- a/MdeModulePkg/Core/PiSmmCore/Smi.c +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c @@ -8,6 +8,11 @@ #include "PiSmmCore.h" +// +// mSmiManageCallingDepth is used to track the depth of recursive calls of= SmiManage. +// +UINTN mSmiManageCallingDepth =3D 0; + LIST_ENTRY mSmiEntryList =3D INITIALIZE_LIST_HEAD_VARIABLE (mSmiEntryList= ); SMI_ENTRY mRootSmiEntry =3D { @@ -79,6 +84,40 @@ SmmCoreFindSmiEntry ( return SmiEntry; } +/** + Remove SmiHandler and free the memory it used. + If SmiEntry is empty, remove SmiEntry and free the memory it used. + + @param SmiHandler Points to SMI handler. + @param SmiEntry Points to SMI Entry or NULL for root SMI handlers. + + @retval TRUE SmiEntry is removed. + @retval FALSE SmiEntry is not removed. +**/ +BOOLEAN +RemoveSmiHandler ( + IN SMI_HANDLER *SmiHandler, + IN SMI_ENTRY *SmiEntry + ) +{ + ASSERT (SmiHandler->ToRemove); + RemoveEntryList (&SmiHandler->Link); + FreePool (SmiHandler); + + // + // Remove the SMI_ENTRY if all handlers have been removed. + // + if (SmiEntry !=3D NULL) { + if (IsListEmpty (&SmiEntry->SmiHandlers)) { + RemoveEntryList (&SmiEntry->AllEntries); + FreePool (SmiEntry); + return TRUE; + } + } + + return FALSE; +} + /** Manage SMI of a particular type. @@ -104,15 +143,17 @@ SmiManage ( { LIST_ENTRY *Link; LIST_ENTRY *Head; + LIST_ENTRY *EntryLink; SMI_ENTRY *SmiEntry; SMI_HANDLER *SmiHandler; - BOOLEAN SuccessReturn; + EFI_STATUS ReturnStatus; + BOOLEAN WillReturn; EFI_STATUS Status; PERF_FUNCTION_BEGIN (); - - Status =3D EFI_NOT_FOUND; - SuccessReturn =3D FALSE; + mSmiManageCallingDepth++; + Status =3D EFI_NOT_FOUND; + ReturnStatus =3D Status; if (HandlerType =3D=3D NULL) { // // Root SMI handler @@ -152,7 +193,16 @@ SmiManage ( // if (HandlerType !=3D NULL) { PERF_FUNCTION_END (); - return EFI_INTERRUPT_PENDING; + ReturnStatus =3D EFI_INTERRUPT_PENDING; + WillReturn =3D TRUE; + } else { + // + // If any other handler's result sets ReturnStatus as EFI_SUCCES= S, the return status + // will be EFI_SUCCESS. + // + if (ReturnStatus !=3D EFI_SUCCESS) { + ReturnStatus =3D Status; + } } break; @@ -165,10 +215,10 @@ SmiManage ( // if (HandlerType !=3D NULL) { PERF_FUNCTION_END (); - return EFI_SUCCESS; + WillReturn =3D TRUE; } - SuccessReturn =3D TRUE; + ReturnStatus =3D EFI_SUCCESS; break; case EFI_WARN_INTERRUPT_SOURCE_QUIESCED: @@ -176,7 +226,7 @@ SmiManage ( // If at least one of the handlers returns EFI_WARN_INTERRUPT_SOUR= CE_QUIESCED // then the function will return EFI_SUCCESS. // - SuccessReturn =3D TRUE; + ReturnStatus =3D EFI_SUCCESS; break; case EFI_WARN_INTERRUPT_SOURCE_PENDING: @@ -184,6 +234,10 @@ SmiManage ( // If all the handlers returned EFI_WARN_INTERRUPT_SOURCE_PENDING // then EFI_WARN_INTERRUPT_SOURCE_PENDING will be returned. // + if (ReturnStatus !=3D EFI_SUCCESS) { + ReturnStatus =3D Status; + } + break; default: @@ -193,14 +247,74 @@ SmiManage ( ASSERT (FALSE); break; } + + if (WillReturn) { + break; + } } - if (SuccessReturn) { - Status =3D EFI_SUCCESS; + ASSERT (mSmiManageCallingDepth > 0); + mSmiManageCallingDepth--; + + // + // SmiHandlerUnRegister() calls from SMI handlers are deferred till this= point. + // Before returned from SmiManage, delete the SmiHandler which is + // marked as ToRemove. + // Note that SmiManage can be called recursively. + // + if (mSmiManageCallingDepth =3D=3D 0) { + // + // Go through all SmiHandler in root SMI handlers + // + for ( Link =3D GetFirstNode (&mRootSmiEntry.SmiHandlers) + ; !IsNull (&mRootSmiEntry.SmiHandlers, Link); + ) + { + // + // SmiHandler might be removed in below, so cache the next link in L= ink + // + SmiHandler =3D CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE); + Link =3D GetNextNode (&mRootSmiEntry.SmiHandlers, Link); + if (SmiHandler->ToRemove) { + // + // Remove SmiHandler if the ToRemove is set. + // + RemoveSmiHandler (SmiHandler, NULL); + } + } + + // + // Go through all SmiHandler in non-root SMI handlers + // + for ( EntryLink =3D GetFirstNode (&mSmiEntryList) + ; !IsNull (&mSmiEntryList, EntryLink); + ) + { + // + // SmiEntry might be removed in below, so cache the next link in Ent= ryLink + // + SmiEntry =3D CR (EntryLink, SMI_ENTRY, AllEntries, SMI_ENTRY_SIGNAT= URE); + EntryLink =3D GetNextNode (&mSmiEntryList, EntryLink); + for ( Link =3D GetFirstNode (&SmiEntry->SmiHandlers) + ; !IsNull (&SmiEntry->SmiHandlers, Link); + ) + { + // + // SmiHandler might be removed in below, so cache the next link in= Link + // + SmiHandler =3D CR (Link, SMI_HANDLER, Link, SMI_HANDLER_SIGNATURE)= ; + Link =3D GetNextNode (&SmiEntry->SmiHandlers, Link); + if (SmiHandler->ToRemove) { + if (RemoveSmiHandler (SmiHandler, SmiEntry)) { + break; + } + } + } + } } PERF_FUNCTION_END (); - return Status; + return ReturnStatus; } /** @@ -238,6 +352,7 @@ SmiHandlerRegister ( SmiHandler->Signature =3D SMI_HANDLER_SIGNATURE; SmiHandler->Handler =3D Handler; SmiHandler->CallerAddr =3D (UINTN)RETURN_ADDRESS (0); + SmiHandler->ToRemove =3D FALSE; if (HandlerType =3D=3D NULL) { // @@ -318,30 +433,27 @@ SmiHandlerUnRegister ( } } - if ((EFI_HANDLE)SmiHandler !=3D DispatchHandle) { + if (SmiHandler =3D=3D NULL) { return EFI_INVALID_PARAMETER; } - SmiEntry =3D SmiHandler->SmiEntry; - - RemoveEntryList (&SmiHandler->Link); - FreePool (SmiHandler); + ASSERT ((EFI_HANDLE)SmiHandler =3D=3D DispatchHandle); + SmiHandler->ToRemove =3D TRUE; - if (SmiEntry =3D=3D NULL) { + if (mSmiManageCallingDepth > 0) { // - // This is root SMI handler + // This function is called from SmiManage() + // Do not delete or remove SmiHandler or SmiEntry now. + // SmiManage will handle it later // return EFI_SUCCESS; } - if (IsListEmpty (&SmiEntry->SmiHandlers)) { - // - // No handler registered for this interrupt now, remove the SMI_ENTRY - // - RemoveEntryList (&SmiEntry->AllEntries); - - FreePool (SmiEntry); - } + SmiEntry =3D SmiHandler->SmiEntry; + // + // For root SMI handler, use NULL for SmiEntry + // + RemoveSmiHandler (SmiHandler, (SmiEntry =3D=3D &mRootSmiEntry) ? NULL : = SmiEntry); return EFI_SUCCESS; } -- 2.31.1.windows.1 -=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 (#117844): https://edk2.groups.io/g/devel/message/117844 Mute This Topic: https://groups.io/mt/105550121/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- --_000_MN6PR11MB8244740C2BDED22FC584CCAD8C082MN6PR11MB8244namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Reviewed-by: Ray Ni <ray.ni@intel.com>



Thanks,
Ray
From: Liu, Zhiguang <zhi= guang.liu@intel.com>
Sent: Tuesday, April 16, 2024 10:41
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Liming Gao <gao= liming@byosoft.com.cn>; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ray = <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH v4 5/6] MdeModulePkg/SMM: Support to unregister SMI = handler in SMI handlers
 
This patch fix a use-after-free issue where unregi= stering an
SMI handler could lead to the deletion of the SMI_HANDLER while it is
still in use by SmiManage(). The fix involves modifying
SmiHandlerUnRegister() to detect whether it is being called from
within the SmiManage() stack. If so, the removal of the SMI_HANDLER
is deferred until SmiManage() has finished executing.
Additionally, due to the possibility of recursive SmiManage() calls,
the unregistration and subsequent removal of the SMI_HANDLER are
ensured to occur only after the outermost SmiManage() invocation has
completed.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h |   1 +
 MdeModulePkg/Core/PiSmmCore/Smi.c      = | 164 ++++++++++++++++++++----
 2 files changed, 139 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/Pi= SmmCore/PiSmmCore.h
index b8a490a8c3..60073c78b4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -93,6 +93,7 @@ typedef struct {
   SMI_ENTRY         = ;            &n= bsp; *SmiEntry;
   VOID         &nbs= p;            &= nbsp;     *Context;    // for profile    UINTN         &nb= sp;            =      ContextSize; // for profile
+  BOOLEAN          =             &nb= sp;  ToRemove;    // To remove this SMI_HANDLER later  } SMI_HANDLER;
 
 //
diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c b/MdeModulePkg/Core/PiSmmCor= e/Smi.c
index 2985f989c3..a84a1f48d3 100644
--- a/MdeModulePkg/Core/PiSmmCore/Smi.c
+++ b/MdeModulePkg/Core/PiSmmCore/Smi.c
@@ -8,6 +8,11 @@
 
 #include "PiSmmCore.h"
 
+//
+// mSmiManageCallingDepth is used to track the depth of recursive calls of= SmiManage.
+//
+UINTN  mSmiManageCallingDepth =3D 0;
+
 LIST_ENTRY  mSmiEntryList =3D INITIALIZE_LIST_HEAD_VARIABLE (mSm= iEntryList);
 
 SMI_ENTRY  mRootSmiEntry =3D {
@@ -79,6 +84,40 @@ SmmCoreFindSmiEntry (
   return SmiEntry;
 }
 
+/**
+  Remove SmiHandler and free the memory it used.
+  If SmiEntry is empty, remove SmiEntry and free the memory it used.<= br> +
+  @param  SmiHandler Points to SMI handler.
+  @param  SmiEntry   Points to SMI Entry or NULL for r= oot SMI handlers.
+
+  @retval TRUE        SmiEntry is = removed.
+  @retval FALSE       SmiEntry is not r= emoved.
+**/
+BOOLEAN
+RemoveSmiHandler (
+  IN SMI_HANDLER  *SmiHandler,
+  IN SMI_ENTRY    *SmiEntry
+  )
+{
+  ASSERT (SmiHandler->ToRemove);
+  RemoveEntryList (&SmiHandler->Link);
+  FreePool (SmiHandler);
+
+  //
+  // Remove the SMI_ENTRY if all handlers have been removed.
+  //
+  if (SmiEntry !=3D NULL) {
+    if (IsListEmpty (&SmiEntry->SmiHandlers)) {
+      RemoveEntryList (&SmiEntry->AllEntri= es);
+      FreePool (SmiEntry);
+      return TRUE;
+    }
+  }
+
+  return FALSE;
+}
+
 /**
   Manage SMI of a particular type.
 
@@ -104,15 +143,17 @@ SmiManage (
 {
   LIST_ENTRY   *Link;
   LIST_ENTRY   *Head;
+  LIST_ENTRY   *EntryLink;
   SMI_ENTRY    *SmiEntry;
   SMI_HANDLER  *SmiHandler;
-  BOOLEAN      SuccessReturn;
+  EFI_STATUS   ReturnStatus;
+  BOOLEAN      WillReturn;
   EFI_STATUS   Status;
 
   PERF_FUNCTION_BEGIN ();
-
-  Status        =3D EFI_NOT_FOUND;=
-  SuccessReturn =3D FALSE;
+  mSmiManageCallingDepth++;
+  Status       =3D EFI_NOT_FOUND;
+  ReturnStatus =3D Status;
   if (HandlerType =3D=3D NULL) {
     //
     // Root SMI handler
@@ -152,7 +193,16 @@ SmiManage (
         //
         if (HandlerType !=3D NULL)= {
           PERF_FUNCTION_= END ();
-          return EFI_INTERRUP= T_PENDING;
+          ReturnStatus =3D EF= I_INTERRUPT_PENDING;
+          WillReturn &nb= sp; =3D TRUE;
+        } else {
+          //
+          // If any other han= dler's result sets ReturnStatus as EFI_SUCCESS, the return status
+          // will be EFI_SUCC= ESS.
+          //
+          if (ReturnStatus != =3D EFI_SUCCESS) {
+            ReturnS= tatus =3D Status;
+          }
         }
 
         break;
@@ -165,10 +215,10 @@ SmiManage (
         //
         if (HandlerType !=3D NULL)= {
           PERF_FUNCTION_= END ();
-          return EFI_SUCCESS;=
+          WillReturn =3D TRUE= ;
         }
 
-        SuccessReturn =3D TRUE;
+        ReturnStatus =3D EFI_SUCCESS;          break;
 
       case EFI_WARN_INTERRUPT_SOURCE_QUIESCE= D:
@@ -176,7 +226,7 @@ SmiManage (
         // If at least one of the = handlers returns EFI_WARN_INTERRUPT_SOURCE_QUIESCED
         // then the function will = return EFI_SUCCESS.
         //
-        SuccessReturn =3D TRUE;
+        ReturnStatus =3D EFI_SUCCESS;          break;
 
       case EFI_WARN_INTERRUPT_SOURCE_PENDING= :
@@ -184,6 +234,10 @@ SmiManage (
         // If all the handlers ret= urned EFI_WARN_INTERRUPT_SOURCE_PENDING
         // then EFI_WARN_INTERRUPT= _SOURCE_PENDING will be returned.
         //
+        if (ReturnStatus !=3D EFI_SUCCE= SS) {
+          ReturnStatus =3D St= atus;
+        }
+
         break;
 
       default:
@@ -193,14 +247,74 @@ SmiManage (
         ASSERT (FALSE);
         break;
     }
+
+    if (WillReturn) {
+      break;
+    }
   }
 
-  if (SuccessReturn) {
-    Status =3D EFI_SUCCESS;
+  ASSERT (mSmiManageCallingDepth > 0);
+  mSmiManageCallingDepth--;
+
+  //
+  // SmiHandlerUnRegister() calls from SMI handlers are deferred till= this point.
+  // Before returned from SmiManage, delete the SmiHandler which is +  // marked as ToRemove.
+  // Note that SmiManage can be called recursively.
+  //
+  if (mSmiManageCallingDepth =3D=3D 0) {
+    //
+    // Go through all SmiHandler in root SMI handlers
+    //
+    for ( Link =3D GetFirstNode (&mRootSmiEntry.SmiHand= lers)
+          ; !IsNull (&mRo= otSmiEntry.SmiHandlers, Link);
+          )
+    {
+      //
+      // SmiHandler might be removed in below, so= cache the next link in Link
+      //
+      SmiHandler =3D CR (Link, SMI_HANDLER, Link,= SMI_HANDLER_SIGNATURE);
+      Link       = =3D GetNextNode (&mRootSmiEntry.SmiHandlers, Link);
+      if (SmiHandler->ToRemove) {
+        //
+        // Remove SmiHandler if the ToR= emove is set.
+        //
+        RemoveSmiHandler (SmiHandler, N= ULL);
+      }
+    }
+
+    //
+    // Go through all SmiHandler in non-root SMI handlers +    //
+    for ( EntryLink =3D GetFirstNode (&mSmiEntryList) +          ; !IsNull (&mSm= iEntryList, EntryLink);
+          )
+    {
+      //
+      // SmiEntry might be removed in below, so c= ache the next link in EntryLink
+      //
+      SmiEntry  =3D CR (EntryLink, SMI_ENTRY= , AllEntries, SMI_ENTRY_SIGNATURE);
+      EntryLink =3D GetNextNode (&mSmiEntryLi= st, EntryLink);
+      for ( Link =3D GetFirstNode (&SmiEntry-= >SmiHandlers)
+            ; !IsNu= ll (&SmiEntry->SmiHandlers, Link);
+            )
+      {
+        //
+        // SmiHandler might be removed = in below, so cache the next link in Link
+        //
+        SmiHandler =3D CR (Link, SMI_HA= NDLER, Link, SMI_HANDLER_SIGNATURE);
+        Link    &nb= sp;  =3D GetNextNode (&SmiEntry->SmiHandlers, Link);
+        if (SmiHandler->ToRemove) {<= br> +          if (RemoveSmiHandle= r (SmiHandler, SmiEntry)) {
+            break;<= br> +          }
+        }
+      }
+    }
   }
 
   PERF_FUNCTION_END ();
-  return Status;
+  return ReturnStatus;
 }
 
 /**
@@ -238,6 +352,7 @@ SmiHandlerRegister (
   SmiHandler->Signature  =3D SMI_HANDLER_SIGNATURE;
   SmiHandler->Handler    =3D Handler;
   SmiHandler->CallerAddr =3D (UINTN)RETURN_ADDRESS (0);
+  SmiHandler->ToRemove   =3D FALSE;
 
   if (HandlerType =3D=3D NULL) {
     //
@@ -318,30 +433,27 @@ SmiHandlerUnRegister (
     }
   }
 
-  if ((EFI_HANDLE)SmiHandler !=3D DispatchHandle) {
+  if (SmiHandler =3D=3D NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  SmiEntry =3D SmiHandler->SmiEntry;
-
-  RemoveEntryList (&SmiHandler->Link);
-  FreePool (SmiHandler);
+  ASSERT ((EFI_HANDLE)SmiHandler =3D=3D DispatchHandle);
+  SmiHandler->ToRemove =3D TRUE;
 
-  if (SmiEntry =3D=3D NULL) {
+  if (mSmiManageCallingDepth > 0) {
     //
-    // This is root SMI handler
+    // This function is called from SmiManage()
+    // Do not delete or remove SmiHandler or SmiEntry now.<= br> +    // SmiManage will handle it later
     //
     return EFI_SUCCESS;
   }
 
-  if (IsListEmpty (&SmiEntry->SmiHandlers)) {
-    //
-    // No handler registered for this interrupt now, remove= the SMI_ENTRY
-    //
-    RemoveEntryList (&SmiEntry->AllEntries);
-
-    FreePool (SmiEntry);
-  }
+  SmiEntry =3D SmiHandler->SmiEntry;
 
+  //
+  // For root SMI handler, use NULL for SmiEntry
+  //
+  RemoveSmiHandler (SmiHandler, (SmiEntry =3D=3D &mRootSmiEntry) = ? NULL : SmiEntry);
   return EFI_SUCCESS;
 }
--
2.31.1.windows.1

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#117844) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_MN6PR11MB8244740C2BDED22FC584CCAD8C082MN6PR11MB8244namp_--