From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2601B21CC534E for ; Thu, 6 Jul 2017 02:19:34 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Jul 2017 02:21:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,316,1496127600"; d="scan'208";a="123040443" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga005.fm.intel.com with ESMTP; 06 Jul 2017 02:21:13 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 6 Jul 2017 02:21:13 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 6 Jul 2017 02:21:13 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.197]) with mapi id 14.03.0319.002; Thu, 6 Jul 2017 17:21:10 +0800 From: "Zeng, Star" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Gao, Liming" , "Wu, Hao A" , "Zeng, Star" Thread-Topic: [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory Thread-Index: AQHS9Xv1kY96XYn1wUOi/qTnZAh/l6JGh1nQ Date: Thu, 6 Jul 2017 09:21:10 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B8F1DBD@shsmsx102.ccr.corp.intel.com> References: <20170705104614.295620-1-ruiyu.ni@intel.com> In-Reply-To: <20170705104614.295620-1-ruiyu.ni@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jul 2017 09:19:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Star Zeng -----Original Message----- From: Ni, Ruiyu=20 Sent: Wednesday, July 5, 2017 6:46 PM To: edk2-devel@lists.01.org Cc: Kinney, Michael D ; Gao, Liming ; Zeng, Star ; Wu, Hao A Subject: [PATCH] MdeModulePkg/DxeCore: Avoid accessing non-owned memory The patch fixes two kinds of issues in DxeCore that accesses memory which m= ight be freed or owned by other modules. The existing bugs don't cause functionality issue. 1. CoreValidateHandle() checks whether the handle is valid by validating its signature. The proper way is to check whether the handle is in the handle database. 2. CoreDisconnectControllersUsingProtocolInterface() and CoreOpenProtocol() de-reference Link pointer which is already freed. The proper way is to not de-reference the pointer. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Cc: Michael D Kinney Cc: Liming Gao Cc: Star Zeng Cc: Hao A Wu --- MdeModulePkg/Core/Dxe/Hand/Handle.c | 54 ++++++++++++++++++---------------= ---- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Ha= nd/Handle.c index 59b89148c8..8e2f361805 100644 --- a/MdeModulePkg/Core/Dxe/Hand/Handle.c +++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c @@ -72,15 +72,20 @@ CoreValidateHandle ( ) { IHANDLE *Handle; + LIST_ENTRY *Link; =20 - Handle =3D (IHANDLE *)UserHandle; - if (Handle =3D=3D NULL) { + if (UserHandle =3D=3D NULL) { return EFI_INVALID_PARAMETER; } - if (Handle->Signature !=3D EFI_HANDLE_SIGNATURE) { - return EFI_INVALID_PARAMETER; + + for (Link =3D gHandleList.ForwardLink; Link !=3D &gHandleList; Link =3D = Link->ForwardLink) { + Handle =3D CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE); + if (Handle =3D=3D (IHANDLE *) UserHandle) { + return EFI_SUCCESS; + } } - return EFI_SUCCESS; + + return EFI_INVALID_PARAMETER; } =20 =20 @@ -643,19 +648,16 @@ CoreDisconnectControllersUsingProtocolInterface ( // do { ItemFound =3D FALSE; - for ( Link =3D Prot->OpenList.ForwardLink; - (Link !=3D &Prot->OpenList) && !ItemFound; - Link =3D Link->ForwardLink ) { + for (Link =3D Prot->OpenList.ForwardLink; Link !=3D &Prot->OpenList;=20 + Link =3D Link->ForwardLink) { OpenData =3D CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_= SIGNATURE); if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) !=3D 0) { - ItemFound =3D TRUE; CoreReleaseProtocolLock (); Status =3D CoreDisconnectController (UserHandle, OpenData->AgentHa= ndle, NULL); CoreAcquireProtocolLock (); - if (EFI_ERROR (Status)) { - ItemFound =3D FALSE; - break; + if (!EFI_ERROR (Status)) { + ItemFound =3D TRUE; } + break; } } } while (ItemFound); @@ -664,21 +666,17 @@ CoreDisconnectControllersUsingProtocolInterface ( // // Attempt to remove BY_HANDLE_PROTOOCL and GET_PROTOCOL and TEST_PROT= OCOL Open List items // - do { - ItemFound =3D FALSE; - for ( Link =3D Prot->OpenList.ForwardLink; - (Link !=3D &Prot->OpenList) && !ItemFound; - Link =3D Link->ForwardLink ) { - OpenData =3D CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DAT= A_SIGNATURE); - if ((OpenData->Attributes & - (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_= PROTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) !=3D 0) { - ItemFound =3D TRUE; - RemoveEntryList (&OpenData->Link); - Prot->OpenListCount--; - CoreFreePool (OpenData); - } + for (Link =3D Prot->OpenList.ForwardLink; Link !=3D &Prot->OpenList;) = { + OpenData =3D CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_DATA_= SIGNATURE); + if ((OpenData->Attributes & + (EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL | EFI_OPEN_PROTOCOL_GET_PR= OTOCOL | EFI_OPEN_PROTOCOL_TEST_PROTOCOL)) !=3D 0) { + Link =3D RemoveEntryList (&OpenData->Link); + Prot->OpenListCount--; + CoreFreePool (OpenData); + } else { + Link =3D Link->ForwardLink; } - } while (ItemFound); + } } =20 // @@ -1132,7 +1130,7 @@ CoreOpenProtocol ( if (ByDriver) { do { Disconnect =3D FALSE; - for ( Link =3D Prot->OpenList.ForwardLink; (Link !=3D &Prot->OpenL= ist) && (!Disconnect); Link =3D Link->ForwardLink) { + for (Link =3D Prot->OpenList.ForwardLink; Link !=3D=20 + &Prot->OpenList; Link =3D Link->ForwardLink) { OpenData =3D CR (Link, OPEN_PROTOCOL_DATA, Link, OPEN_PROTOCOL_D= ATA_SIGNATURE); if ((OpenData->Attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) !=3D 0)= { Disconnect =3D TRUE; @@ -1142,6 +1140,8 @@ CoreOpenProtocol ( if (EFI_ERROR (Status)) { Status =3D EFI_ACCESS_DENIED; goto Done; + } else { + break; } } } -- 2.12.2.windows.2