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 008A9740032 for ; Tue, 16 Apr 2024 03:52:50 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=mv7Xsibu4zPPui25VdCaLahr/3aMIMOH+DT14dfECz0=; 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=Z16yIuW3AL5CH6/jyqpvW4IZTQPvnrDnx7h0QvJ2WcHH6iv2zUkHV96T0cbWvq1dmbMlegsv bg74i15LY77/I78VOsZxF/N2njqb/BSYpUAmsrAZnb3T42dsrvRx9KeBN7Ol4rCUA5ivLSdYJ5c mleSKXfjFsSUnctoW4kC4JJlfBcaC+c9l0D7HC3OUYXF0SARIjYGvCvY0sblQeITtuDIuc/6CGT RPGQLBJ+4y2eEZFLRFPG71q513RvDZQJu/tWZtGXG+zKA+VLe78RTBJdcmCcA6ytiQfIChxj+VQ yMQiD8+jl401ufxL+89mUam4NsdzVbQf3RW0FZGG6v3Xg== X-Received: by 127.0.0.2 with SMTP id MT9lYY7687511xA3vBb6UoO9; Mon, 15 Apr 2024 20:52:49 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.12]) by mx.groups.io with SMTP id smtpd.web10.12028.1713239563832599145 for ; Mon, 15 Apr 2024 20:52:44 -0700 X-CSE-ConnectionGUID: 4IapurdlQJWazK+IQxEqQg== X-CSE-MsgGUID: V1gKrYpESuOlYqPdEXQf1g== X-IronPort-AV: E=McAfee;i="6600,9927,11045"; a="20078277" X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="20078277" X-Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by orvoesa104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2024 20:52:43 -0700 X-CSE-ConnectionGUID: IbC21vAlS3y/X6ATXz8/Lg== X-CSE-MsgGUID: XacEtsekQJWd82clJU4TbA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.07,204,1708416000"; d="scan'208,217";a="22197918" X-Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmviesa006.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 15 Apr 2024 20:52:43 -0700 X-Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX603.amr.corp.intel.com (10.22.229.16) 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:41 -0700 X-Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by orsmsx610.amr.corp.intel.com (10.22.229.23) 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:41 -0700 X-Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.169) by edgegateway.intel.com (134.134.137.100) 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:41 -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:39 +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:39 +0000 From: "Ni, Ray" To: "Liu, Zhiguang" , "devel@edk2.groups.io" CC: Liming Gao , "Wu, Jiaxin" , Laszlo Ersek , Ard Biesheuvel , Sami Mujawar Subject: Re: [edk2-devel] [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Thread-Topic: [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler in MMI handlers Thread-Index: AQHaj6ebtxDmKha+rkiiZJVfD50C57FqQ6OH Date: Tue, 16 Apr 2024 03:52:39 +0000 Message-ID: References: <20240416024108.1358-1-zhiguang.liu@intel.com> <20240416024108.1358-7-zhiguang.liu@intel.com> In-Reply-To: <20240416024108.1358-7-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: 3882f10b-9d1b-442e-654e-08dc5dc8a98a x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: 1LZiVAsOYsC78P2gGSSoBE2mCiILc0VtiZGXZUP0USj85OpATKbVNRapVrqhZjoqum/+piCToNggzgJmsi9mXxbCGdsI9prfyL0btjkKaPCpQcT04l7Uo0pHSNGbRa3neHeNbtXrr90ye50EsNgQyUybB9T+5maF3SW/iwmLMsAjNQO03KnI7JDyJxFLsyJMbj3pKUqCzC7KeQiF28/O6IsNIDVx2t08zDE+VNCNtsbWLjiCOoW9fi3ZPBWAZg5+5c4IInfgsxI+qQpUVsaCUQBOphB7cBOoMHgBDf8z60Q8aLYmUcEgZCJzjEphqzEKXReY48eN/7ubC5YMiKWtXARXhVXmqFLRRT7gu850sBVu+HdC/ijKVZpJbzk7xi+tl/7GWQ8evD+IslCtiOAUabwf1S0NMkER8uwG7A3RFEYzfKgDcBeL21HEDh+wQNIlofByRG/0JbXFoX1KjF2dp9aagdEcqAIMIjN60FvTVQ6bRopUKb62iGZhXmhFcrSu2JMFSxfL17COYHkNgd9fI4Be0Qm+CfWtyC2fjKgfMm6Oo5dNyKQwnkxp9BBshzVf0R0+kGnJIMljKA0z9jFuYDsNkZnOn3dYxtHItPEMyhgJ/7/CHSWQIpC9d51X5zbWl27K/He5NQM3GQrVESM7W+Sdn0mEjBeiMtkguxc+vuPY+JrDCQva7YrMxoFfn1YqwH/s68hhWrO1I2RYNjln29Ut9VlWRV+875lb9kvtMdk= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?0m7nuzmnGFHAxy5Rr3aid3sJKnChdpY7tpjAffTtCy6Ju5He9xNfVbpLDpy8?= =?us-ascii?Q?JPXdcgUizt7Ngi8+ibmWwtti5CjXzeCSKLv7VzWvu1d4dUJs05gkahjAgyEk?= =?us-ascii?Q?K3vzsl146KDWmKTcLXrVeCCiL3ZR7/a1/cyFW+hc6m7F7vEqGv+fiBCxpKHc?= =?us-ascii?Q?zCsVu6lU2ia2GDp422fGqJz7OQojyiaAky938PdEOcavSCwmDU4lbmLHwSiN?= =?us-ascii?Q?UaV11EdoipSTMyQjieDVD7MMjPYJ3uy4wEl9l6KBeeqBtVWXhMiruVaK+vbC?= =?us-ascii?Q?RoFXvQWNeTmz/tyvf6B0lYWe0BcjVNNLrwhpfsWP2bm8i+EI6FEiPP7lgDuE?= =?us-ascii?Q?jikdwp9huLztnCk/nnRdd5/h5GNcv8RXr8D9wj/GQOh29/6yLesUZ9iwuz3f?= =?us-ascii?Q?OLMjYLWiS5TOZcdLIK3MNA0w4vYTo491iOKjYXB3f9Qe/da2cX235Y0wPoNZ?= =?us-ascii?Q?/D6x/ZXsvV0Xd8d0nfc86NLHB5xbS5HAjOarZtTaWhjbRpHFXzP0kTfDMu1u?= =?us-ascii?Q?Tql6Nzi3GgzzPyTuFR8YWMKS+d6kAEAR0TA3WLMfj8Wfhwfa0wHxGmGXcEPY?= =?us-ascii?Q?/ejjZ+bI2Cvt8xphnDMAXx2Vk4kjGKoc1+Q2RIWtJ7L1Jfy8nqXLxgGlROpk?= =?us-ascii?Q?/bsoQVAHc+lqZdeAiXnNxraeyUkRoksZCNEnHJYzWDqY65xhYzpwY7domcYc?= =?us-ascii?Q?WITXjfEoUbqnrETtrgA2ucIg5R2/ilsgL5ZqaqA+p8A8iKXPWBnhnYzcAvXo?= =?us-ascii?Q?fFi3jYrSjWGiT3jNrUXf4xi/DMjlFsjclLHtYHLbl90JFWpHsHN/t9JkAeUH?= =?us-ascii?Q?ufcBLdW7LhaW04sYOhdh16lRoaFDf6w4I4DCNv74+SE3Eitw5Cn0amxrIy0D?= =?us-ascii?Q?+J917Qw1m+wz38TzPWTiHHVNH+uaZZ3cr+1kIy8p3YVKxqKYvmScvSpcxyYv?= =?us-ascii?Q?G+bFJR8t0nd/kegpOGi/f2AIRcWSKVTMSQ40xmANSLRF1PP1Mmk4jCrUvHz/?= =?us-ascii?Q?oM4nNRQpeiNtEi3sLVkOz6FK/fyKDdn7VOpYxJ6EPjmWmwHofMlboygnKUOd?= =?us-ascii?Q?8ya/L66/Be+tGc+5F7FD8FZqRFyUJDenn7DZiUD6UWlXMlVsJrGhcnl4HB3y?= =?us-ascii?Q?EQ9AwCfuZR3GuNHiLxjsx+Yv1kCe4yulUxnOTk+hkzRExohblgtN/NCC/1K+?= =?us-ascii?Q?9mLqBvfU4WJQj2KeZbguekGmeYvA18Wuun4W90+z1wxH/8afJugWiYAqmHau?= =?us-ascii?Q?ZYaufluatQkAep2rKXCqWGxOSDfwYXA4kfS/UPZiMxOZv5kNXz2o+e483MEh?= =?us-ascii?Q?/t8DJhSB4XuxGTXorVGd1hze3/mSGf4xbDohXKB3/kbHunOIQoPaclcpBydP?= =?us-ascii?Q?NC5LnOqsmzLOnahMX6L5vxouvn3gTI4kDCz9wSlglV3ItLJBo1UEujVyuBYA?= =?us-ascii?Q?8YyENYq4VhcIdqSyETfqlhq02oehxm3Kc/UM8QlD2kp64OjDlozdsoOs4qBz?= =?us-ascii?Q?qsRt1WhDPzLkBqG7XlHe9I5mebmVlKnMsIMK4CeDVNbHhPoxBpC5zowHpcWe?= =?us-ascii?Q?I0x/xJclBOo7h3lSRTc=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: 3882f10b-9d1b-442e-654e-08dc5dc8a98a X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 03:52:39.8871 (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: HSroE5x/LG3YOBa5xkvUxZPWlfmfh0xJ3OW5sxuqxjwv0bRhIGwOHYKqkH7EKq45RRNm6lajB/+lrQv3RA2XBQ== 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:44 -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: 3RxDsRuS4mQalwEFDITvithux7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244477F43A77A889C9A597A8C082MN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=Z16yIuW3; 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_MN6PR11MB8244477F43A77A889C9A597A8C082MN6PR11MB8244namp_ 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 ; Ard Biesheuvel ; Sa= mi Mujawar Subject: [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI handler = in MMI handlers This patch fix a use-after-free issue where unregistering an MMI handler could lead to the deletion of the MMI_HANDLER while it is still in use by MmiManage(). The fix involves modifying MmiHandlerUnRegister() to detect whether it is being called from within the MmiManage() stack. If so, the removal of the MMI_HANDLER is deferred until MmiManage() has finished executing. Additionally, due to the possibility of recursive MmiManage() calls, the unregistration and subsequent removal of the MMI_HANDLER are ensured to occur only after the outermost MmiManage() invocation has completed. Cc: Liming Gao Cc: Jiaxin Wu Cc: Ray Ni Cc: Laszlo Ersek Cc: Ard Biesheuvel Cc: Sami Mujawar Signed-off-by: Zhiguang Liu --- StandaloneMmPkg/Core/Mmi.c | 161 +++++++++++++++++++++++++++++++------ 1 file changed, 136 insertions(+), 25 deletions(-) diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c index 0de6fd17fc..e035245c87 100644 --- a/StandaloneMmPkg/Core/Mmi.c +++ b/StandaloneMmPkg/Core/Mmi.c @@ -34,11 +34,51 @@ typedef struct { LIST_ENTRY Link; // Link on MMI_ENTRY.MmiHandl= ers EFI_MM_HANDLER_ENTRY_POINT Handler; // The mm handler's entry poi= nt MMI_ENTRY *MmiEntry; + BOOLEAN ToRemove; // To remove this MMI_HANDLER= later } MMI_HANDLER; +// +// mMmiManageCallingDepth is used to track the depth of recursive calls of= MmiManage. +// +UINTN mMmiManageCallingDepth =3D 0; + LIST_ENTRY mRootMmiHandlerList =3D INITIALIZE_LIST_HEAD_VARIABLE (mRootMm= iHandlerList); LIST_ENTRY mMmiEntryList =3D INITIALIZE_LIST_HEAD_VARIABLE (mMmiEnt= ryList); +/** + Remove MmiHandler and free the memory it used. + If MmiEntry is empty, remove MmiEntry and free the memory it used. + + @param MmiHandler Points to MMI handler. + @param MmiEntry Points to MMI Entry or NULL for root MMI handlers. + + @retval TRUE MmiEntry is removed. + @retval FALSE MmiEntry is not removed. +**/ +BOOLEAN +RemoveMmiHandler ( + IN MMI_HANDLER *MmiHandler, + IN MMI_ENTRY *MmiEntry + ) +{ + ASSERT (MmiHandler->ToRemove); + RemoveEntryList (&MmiHandler->Link); + FreePool (MmiHandler); + + // + // Remove the MMI_ENTRY if all handlers have been removed. + // + if (MmiEntry !=3D NULL) { + if (IsListEmpty (&MmiEntry->MmiHandlers)) { + RemoveEntryList (&MmiEntry->AllEntries); + FreePool (MmiEntry); + return TRUE; + } + } + + return FALSE; +} + /** Finds the MMI entry for the requested handler type. @@ -126,13 +166,16 @@ MmiManage ( { LIST_ENTRY *Link; LIST_ENTRY *Head; + LIST_ENTRY *EntryLink; MMI_ENTRY *MmiEntry; MMI_HANDLER *MmiHandler; - BOOLEAN SuccessReturn; + EFI_STATUS ReturnStatus; + BOOLEAN WillReturn; EFI_STATUS Status; - Status =3D EFI_NOT_FOUND; - SuccessReturn =3D FALSE; + mMmiManageCallingDepth++; + Status =3D EFI_NOT_FOUND; + ReturnStatus =3D Status; if (HandlerType =3D=3D NULL) { // // Root MMI handler @@ -171,7 +214,16 @@ MmiManage ( // no additional handlers will be processed and EFI_INTERRUPT_PEND= ING will be returned. // if (HandlerType !=3D NULL) { - 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; @@ -183,10 +235,10 @@ MmiManage ( // additional handlers will be processed. // if (HandlerType !=3D NULL) { - return EFI_SUCCESS; + WillReturn =3D TRUE; } - SuccessReturn =3D TRUE; + ReturnStatus =3D EFI_SUCCESS; break; case EFI_WARN_INTERRUPT_SOURCE_QUIESCED: @@ -194,7 +246,7 @@ MmiManage ( // 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: @@ -202,6 +254,10 @@ MmiManage ( // 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: @@ -211,13 +267,76 @@ MmiManage ( ASSERT_EFI_ERROR (Status); break; } + + if (WillReturn) { + break; + } } - if (SuccessReturn) { - Status =3D EFI_SUCCESS; + ASSERT (mMmiManageCallingDepth > 0); + mMmiManageCallingDepth--; + + // + // MmiHandlerUnRegister() calls from MMI handlers are deferred till this= point. + // Before returned from MmiManage, delete the MmiHandler which is + // marked as ToRemove. + // Note that MmiManage can be called recursively. + // + if (mMmiManageCallingDepth =3D=3D 0) { + // + // Go through all MmiHandler in root Mmi handlers + // + for ( Link =3D GetFirstNode (&mRootMmiHandlerList) + ; !IsNull (&mRootMmiHandlerList, Link); + ) + { + // + // MmiHandler might be removed in below, so cache the next link in L= ink + // + MmiHandler =3D CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE); + Link =3D GetNextNode (&mRootMmiHandlerList, Link); + if (MmiHandler->ToRemove) { + // + // Remove MmiHandler if the ToRemove is set. + // + RemoveMmiHandler (MmiHandler, NULL); + } + } + + // + // Go through all MmiHandler in non-root MMI handlers + // + for ( EntryLink =3D GetFirstNode (&mMmiEntryList) + ; !IsNull (&mMmiEntryList, EntryLink); + ) + { + // + // MmiEntry might be removed in below, so cache the next link in Ent= ryLink + // + MmiEntry =3D CR (EntryLink, MMI_ENTRY, AllEntries, MMI_ENTRY_SIGNAT= URE); + EntryLink =3D GetNextNode (&mMmiEntryList, EntryLink); + for ( Link =3D GetFirstNode (&MmiEntry->MmiHandlers) + ; !IsNull (&MmiEntry->MmiHandlers, Link); + ) + { + // + // MmiHandler might be removed in below, so cache the next link in= Link + // + MmiHandler =3D CR (Link, MMI_HANDLER, Link, MMI_HANDLER_SIGNATURE)= ; + Link =3D GetNextNode (&MmiEntry->MmiHandlers, Link); + if (MmiHandler->ToRemove) { + // + // Remove MmiHandler if the ToRemove is set. + // + if (RemoveMmiHandler (MmiHandler, MmiEntry)) { + break; + } + } + } + } } - return Status; + return ReturnStatus; } /** @@ -254,6 +373,7 @@ MmiHandlerRegister ( MmiHandler->Signature =3D MMI_HANDLER_SIGNATURE; MmiHandler->Handler =3D Handler; + MmiHandler->ToRemove =3D FALSE; if (HandlerType =3D=3D NULL) { // @@ -309,26 +429,17 @@ MmiHandlerUnRegister ( return EFI_INVALID_PARAMETER; } - MmiEntry =3D MmiHandler->MmiEntry; - - RemoveEntryList (&MmiHandler->Link); - FreePool (MmiHandler); - - if (MmiEntry =3D=3D NULL) { + MmiHandler->ToRemove =3D TRUE; + if (mMmiManageCallingDepth > 0) { // - // This is root MMI handler + // This function is called from MmiManage() + // Do not delete or remove MmiHandler or MmiEntry now. // return EFI_SUCCESS; } - if (IsListEmpty (&MmiEntry->MmiHandlers)) { - // - // No handler registered for this interrupt now, remove the MMI_ENTRY - // - RemoveEntryList (&MmiEntry->AllEntries); - - FreePool (MmiEntry); - } + MmiEntry =3D MmiHandler->MmiEntry; + RemoveMmiHandler (MmiHandler, MmiEntry); 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 (#117843): https://edk2.groups.io/g/devel/message/117843 Mute This Topic: https://groups.io/mt/105550122/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_MN6PR11MB8244477F43A77A889C9A597A8C082MN6PR11MB8244namp_ 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>; Ard Biesh= euvel <ardb+tianocore@kernel.org>; Sami Mujawar <sami.mujawar@arm.= com>
Subject: [PATCH v4 6/6] StandaloneMmPkg: Support to unregister MMI h= andler in MMI handlers
 
This patch fix a use-after-free issue where unregi= stering an
MMI handler could lead to the deletion of the MMI_HANDLER while it is
still in use by MmiManage(). The fix involves modifying
MmiHandlerUnRegister() to detect whether it is being called from
within the MmiManage() stack. If so, the removal of the MMI_HANDLER
is deferred until MmiManage() has finished executing.
Additionally, due to the possibility of recursive MmiManage() calls,
the unregistration and subsequent removal of the MMI_HANDLER are
ensured to occur only after the outermost MmiManage() 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>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 StandaloneMmPkg/Core/Mmi.c | 161 +++++++++++++++++++++++++++++++-----= -
 1 file changed, 136 insertions(+), 25 deletions(-)

diff --git a/StandaloneMmPkg/Core/Mmi.c b/StandaloneMmPkg/Core/Mmi.c
index 0de6fd17fc..e035245c87 100644
--- a/StandaloneMmPkg/Core/Mmi.c
+++ b/StandaloneMmPkg/Core/Mmi.c
@@ -34,11 +34,51 @@ typedef struct {
   LIST_ENTRY        &nbs= p;           Link; &= nbsp;      // Link on MMI_ENTRY.MmiHandlers
   EFI_MM_HANDLER_ENTRY_POINT    Handler; &nb= sp;   // The mm handler's entry point
   MMI_ENTRY         = ;            *MmiEnt= ry;
+  BOOLEAN          =              To= Remove;    // To remove this MMI_HANDLER later
 } MMI_HANDLER;
 
+//
+// mMmiManageCallingDepth is used to track the depth of recursive calls of= MmiManage.
+//
+UINTN  mMmiManageCallingDepth =3D 0;
+
 LIST_ENTRY  mRootMmiHandlerList =3D INITIALIZE_LIST_HEAD_VARIABL= E (mRootMmiHandlerList);
 LIST_ENTRY  mMmiEntryList       = =3D INITIALIZE_LIST_HEAD_VARIABLE (mMmiEntryList);
 
+/**
+  Remove MmiHandler and free the memory it used.
+  If MmiEntry is empty, remove MmiEntry and free the memory it used.<= br> +
+  @param  MmiHandler  Points to MMI handler.
+  @param  MmiEntry    Points to MMI Entry or NULL= for root MMI handlers.
+
+  @retval TRUE        MmiEntry is = removed.
+  @retval FALSE       MmiEntry is not r= emoved.
+**/
+BOOLEAN
+RemoveMmiHandler (
+  IN MMI_HANDLER  *MmiHandler,
+  IN MMI_ENTRY    *MmiEntry
+  )
+{
+  ASSERT (MmiHandler->ToRemove);
+  RemoveEntryList (&MmiHandler->Link);
+  FreePool (MmiHandler);
+
+  //
+  // Remove the MMI_ENTRY if all handlers have been removed.
+  //
+  if (MmiEntry !=3D NULL) {
+    if (IsListEmpty (&MmiEntry->MmiHandlers)) {
+      RemoveEntryList (&MmiEntry->AllEntri= es);
+      FreePool (MmiEntry);
+      return TRUE;
+    }
+  }
+
+  return FALSE;
+}
+
 /**
   Finds the MMI entry for the requested handler type.
 
@@ -126,13 +166,16 @@ MmiManage (
 {
   LIST_ENTRY   *Link;
   LIST_ENTRY   *Head;
+  LIST_ENTRY   *EntryLink;
   MMI_ENTRY    *MmiEntry;
   MMI_HANDLER  *MmiHandler;
-  BOOLEAN      SuccessReturn;
+  EFI_STATUS   ReturnStatus;
+  BOOLEAN      WillReturn;
   EFI_STATUS   Status;
 
-  Status        =3D EFI_NOT_FOUND;=
-  SuccessReturn =3D FALSE;
+  mMmiManageCallingDepth++;
+  Status       =3D EFI_NOT_FOUND;
+  ReturnStatus =3D Status;
   if (HandlerType =3D=3D NULL) {
     //
     // Root MMI handler
@@ -171,7 +214,16 @@ MmiManage (
         // no additional handlers = will be processed and EFI_INTERRUPT_PENDING will be returned.
         //
         if (HandlerType !=3D NULL)= {
-          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;
@@ -183,10 +235,10 @@ MmiManage (
         // additional handlers wil= l be processed.
         //
         if (HandlerType !=3D NULL)= {
-          return EFI_SUCCESS;=
+          WillReturn =3D TRUE= ;
         }
 
-        SuccessReturn =3D TRUE;
+        ReturnStatus =3D EFI_SUCCESS;          break;
 
       case EFI_WARN_INTERRUPT_SOURCE_QUIESCE= D:
@@ -194,7 +246,7 @@ MmiManage (
         // 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= :
@@ -202,6 +254,10 @@ MmiManage (
         // 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:
@@ -211,13 +267,76 @@ MmiManage (
         ASSERT_EFI_ERROR (Status);=
         break;
     }
+
+    if (WillReturn) {
+      break;
+    }
   }
 
-  if (SuccessReturn) {
-    Status =3D EFI_SUCCESS;
+  ASSERT (mMmiManageCallingDepth > 0);
+  mMmiManageCallingDepth--;
+
+  //
+  // MmiHandlerUnRegister() calls from MMI handlers are deferred till= this point.
+  // Before returned from MmiManage, delete the MmiHandler which is +  // marked as ToRemove.
+  // Note that MmiManage can be called recursively.
+  //
+  if (mMmiManageCallingDepth =3D=3D 0) {
+    //
+    // Go through all MmiHandler in root Mmi handlers
+    //
+    for ( Link =3D GetFirstNode (&mRootMmiHandlerList)<= br> +          ; !IsNull (&mRo= otMmiHandlerList, Link);
+          )
+    {
+      //
+      // MmiHandler might be removed in below, so= cache the next link in Link
+      //
+      MmiHandler =3D CR (Link, MMI_HANDLER, Link,= MMI_HANDLER_SIGNATURE);
+      Link       = =3D GetNextNode (&mRootMmiHandlerList, Link);
+      if (MmiHandler->ToRemove) {
+        //
+        // Remove MmiHandler if the ToR= emove is set.
+        //
+        RemoveMmiHandler (MmiHandler, N= ULL);
+      }
+    }
+
+    //
+    // Go through all MmiHandler in non-root MMI handlers +    //
+    for ( EntryLink =3D GetFirstNode (&mMmiEntryList) +          ; !IsNull (&mMm= iEntryList, EntryLink);
+          )
+    {
+      //
+      // MmiEntry might be removed in below, so c= ache the next link in EntryLink
+      //
+      MmiEntry  =3D CR (EntryLink, MMI_ENTRY= , AllEntries, MMI_ENTRY_SIGNATURE);
+      EntryLink =3D GetNextNode (&mMmiEntryLi= st, EntryLink);
+      for ( Link =3D GetFirstNode (&MmiEntry-= >MmiHandlers)
+            ; !IsNu= ll (&MmiEntry->MmiHandlers, Link);
+            )
+      {
+        //
+        // MmiHandler might be removed = in below, so cache the next link in Link
+        //
+        MmiHandler =3D CR (Link, MMI_HA= NDLER, Link, MMI_HANDLER_SIGNATURE);
+        Link    &nb= sp;  =3D GetNextNode (&MmiEntry->MmiHandlers, Link);
+        if (MmiHandler->ToRemove) {<= br> +          //
+          // Remove MmiHandle= r if the ToRemove is set.
+          //
+          if (RemoveMmiHandle= r (MmiHandler, MmiEntry)) {
+            break;<= br> +          }
+        }
+      }
+    }
   }
 
-  return Status;
+  return ReturnStatus;
 }
 
 /**
@@ -254,6 +373,7 @@ MmiHandlerRegister (
 
   MmiHandler->Signature =3D MMI_HANDLER_SIGNATURE;
   MmiHandler->Handler   =3D Handler;
+  MmiHandler->ToRemove  =3D FALSE;
 
   if (HandlerType =3D=3D NULL) {
     //
@@ -309,26 +429,17 @@ MmiHandlerUnRegister (
     return EFI_INVALID_PARAMETER;
   }
 
-  MmiEntry =3D MmiHandler->MmiEntry;
-
-  RemoveEntryList (&MmiHandler->Link);
-  FreePool (MmiHandler);
-
-  if (MmiEntry =3D=3D NULL) {
+  MmiHandler->ToRemove =3D TRUE;
+  if (mMmiManageCallingDepth > 0) {
     //
-    // This is root MMI handler
+    // This function is called from MmiManage()
+    // Do not delete or remove MmiHandler or MmiEntry now.<= br>      //
     return EFI_SUCCESS;
   }
 
-  if (IsListEmpty (&MmiEntry->MmiHandlers)) {
-    //
-    // No handler registered for this interrupt now, remove= the MMI_ENTRY
-    //
-    RemoveEntryList (&MmiEntry->AllEntries);
-
-    FreePool (MmiEntry);
-  }
+  MmiEntry =3D MmiHandler->MmiEntry;
+  RemoveMmiHandler (MmiHandler, MmiEntry);
 
   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 (#117843) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_MN6PR11MB8244477F43A77A889C9A597A8C082MN6PR11MB8244namp_--