From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=steven.shi@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (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 24D2921E0BA0C for ; Thu, 1 Feb 2018 21:35:15 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Feb 2018 21:40:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,447,1511856000"; d="scan'208";a="14775323" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga007.fm.intel.com with ESMTP; 01 Feb 2018 21:40:53 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 1 Feb 2018 21:40:53 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 1 Feb 2018 21:40:52 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.116]) with mapi id 14.03.0319.002; Fri, 2 Feb 2018 13:40:51 +0800 From: "Shi, Steven" To: Laszlo Ersek , "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Zeng, Star" Thread-Topic: [edk2] [PATCH] MdeModulePkg/SmmCore: Fix hang due to already-freed memory deference Thread-Index: AQHTm0WuEVqIkFK5vEyqOwR5qrC1RqOPMjcAgAFe/DA= Date: Fri, 2 Feb 2018 05:40:50 +0000 Message-ID: <06C8AB66E78EE34A949939824ABE2B313B627EAF@shsmsx102.ccr.corp.intel.com> References: <20180201101539.320452-1-ruiyu.ni@intel.com> <1bde96c6-7ca3-0ee8-9990-6e0ca17026fe@redhat.com> In-Reply-To: <1bde96c6-7ca3-0ee8-9990-6e0ca17026fe@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTA1MTVmZTctZTQ5My00YTNmLWE0YjUtMGI4YzViZDc3N2RiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJtdGt1Y0VTdnZiWFY1UHBXUFwvVVQ4YVY2NmtJc2lERlBXRFNPZUdkUEtIR2tnSWxZOTB1bDZlNWZweUIrUnVrTSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/SmmCore: Fix hang due to already-freed memory deference X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Feb 2018 05:35:16 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, Check the DispatchHandle valid in internal handle set before using it to re= ference its Signature data is majorly to avoid use-after-free problem here,= it also can defense if an input handle is invalid but has a valid signatur= e occasionally or deliberately.=20 > Generally, if client code violates an interface contract, then the called= function is not responsible for catching the error and preventing undefine= d behavior.=20 > For "quality of service", we can go to certain lengths nonetheless, but i= t should hopefully not hurt valid client code. If the called function is an interface function, I think it is necessary to= validate the inputs before use them to reference other internal data. This= way can make the service code more secure. Steven Shi Intel\SSG\STO\UEFI Firmware Tel: +86 021-61166522 iNet: 821-6522 > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, February 2, 2018 12:12 AM > To: Ni, Ruiyu ; edk2-devel@lists.01.org > Cc: Yao, Jiewen ; Zeng, Star > Subject: Re: [edk2] [PATCH] MdeModulePkg/SmmCore: Fix hang due to > already-freed memory deference >=20 > Hello Ray, >=20 > On 02/01/18 11:15, Ruiyu Ni wrote: > > SmiHandlerUnRegister() validates the DispatchHandle by checking > > whether the first 32bit matches to a certain signature > > (SMI_HANDLER_SIGNATURE). > > But if a caller calls *UnRegister() twice and the memory freed by > > first call still contains the signature, the second hang may hang. > > > > The patch fixes this issue by locating the DispatchHandle > > in all SMI handlers, instead of checking the signature. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ruiyu Ni > > Cc: Jiewen Yao > > Cc: Star Zeng > > --- > > MdeModulePkg/Core/PiSmmCore/Smi.c | 37 > ++++++++++++++++++++++++++++++++----- > > 1 file changed, 32 insertions(+), 5 deletions(-) >=20 > I'm mildly curious: can we just zero out the signature when the > de-registration / freeing happens? Otherwise, the nested loop added > below will penalize (performance-wise) correctly written client code as > well. >=20 > > diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c > > b/MdeModulePkg/Core/PiSmmCore/Smi.c > > index ad483a1877ce..6596ea9560d1 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/Smi.c > > +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c > > @@ -290,6 +290,7 @@ SmiHandlerUnRegister ( > > SmiEntry =3D SmiHandler->SmiEntry; > > > > RemoveEntryList (&SmiHandler->Link); > > + SmiHandler->Signature =3D 0; > > FreePool (SmiHandler); > > > > if (SmiEntry =3D=3D NULL) { >=20 > Generally, if client code violates an interface contract, then the > called function is not responsible for catching the error and preventing > undefined behavior. For "quality of service", we can go to certain > lengths nonetheless, but it should hopefully not hurt valid client code. >=20 > For example, I seem to remember that the list data structure > implementation checks the internal consistency (which can be costly) > only if a PCD is set to a certain value. Is that right? Is it an option > here? (If the above zeroing is not good for some reason.) >=20 > Anyway, I'm asking mainly for my own education. >=20 > Thanks! > Laszlo >=20 > > diff --git a/MdeModulePkg/Core/PiSmmCore/Smi.c > b/MdeModulePkg/Core/PiSmmCore/Smi.c > > index ad483a1877..0c09e7fa10 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/Smi.c > > +++ b/MdeModulePkg/Core/PiSmmCore/Smi.c > > @@ -1,7 +1,7 @@ > > /** @file > > SMI management. > > > > - Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved. > > + Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. > > This program and the accompanying materials are licensed and made > available > > under the terms and conditions of the BSD License which accompanies > this > > distribution. The full text of the license may be found at > > @@ -276,14 +276,41 @@ SmiHandlerUnRegister ( > > { > > SMI_HANDLER *SmiHandler; > > SMI_ENTRY *SmiEntry; > > + LIST_ENTRY *EntryLink; > > + LIST_ENTRY *HandlerLink; > > > > - SmiHandler =3D (SMI_HANDLER *) DispatchHandle; > > - > > - if (SmiHandler =3D=3D NULL) { > > + if (DispatchHandle =3D=3D NULL) { > > return EFI_INVALID_PARAMETER; > > } > > > > - if (SmiHandler->Signature !=3D SMI_HANDLER_SIGNATURE) { > > + // > > + // Look for it in root SMI handlers > > + // > > + SmiHandler =3D NULL; > > + for ( HandlerLink =3D GetFirstNode (&mRootSmiEntry.SmiHandlers) > > + ; !IsNull (&mRootSmiEntry.SmiHandlers, HandlerLink) && > (SmiHandler !=3D DispatchHandle) > > + ; HandlerLink =3D GetNextNode (&mRootSmiEntry.SmiHandlers, > HandlerLink) > > + ) { > > + SmiHandler =3D CR (HandlerLink, SMI_HANDLER, Link, > SMI_HANDLER_SIGNATURE); > > + } > > + > > + // > > + // Look for it in non-root SMI handlers > > + // > > + for ( EntryLink =3D GetFirstNode (&mSmiEntryList) > > + ; !IsNull (&mSmiEntryList, EntryLink) && (SmiHandler !=3D > DispatchHandle) > > + ; EntryLink =3D GetNextNode (&mSmiEntryList, EntryLink) > > + ) { > > + SmiEntry =3D CR (EntryLink, SMI_ENTRY, AllEntries, > SMI_ENTRY_SIGNATURE); > > + for ( HandlerLink =3D GetFirstNode (&SmiEntry->SmiHandlers) > > + ; !IsNull (&SmiEntry->SmiHandlers, HandlerLink) && (SmiHandler= !=3D > DispatchHandle) > > + ; HandlerLink =3D GetNextNode (&SmiEntry->SmiHandlers, Handler= Link) > > + ) { > > + SmiHandler =3D CR (HandlerLink, SMI_HANDLER, Link, > SMI_HANDLER_SIGNATURE); > > + } > > + } > > + > > + if (SmiHandler !=3D DispatchHandle) { > > return EFI_INVALID_PARAMETER; > > } > > > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel