From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web10.3372.1610596569335423266 for ; Wed, 13 Jan 2021 19:56:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=PBNNupLp; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.42/8.16.0.42) with SMTP id 10E3qmRH057206; Wed, 13 Jan 2021 19:56:08 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=Fx17czCCPr735jNoLEfPEd6gswwQyrLd5UVyzKibNbQ=; b=PBNNupLp/xMkxYX/7p+4z8ld8r9tzTICTXchbUXjF4J91yTiLAcJKPAjKvP0N2UO+y8J reZtTd/bun/sUmbgOXqPeSGE47dYgtxLzfVpEl5GagCWYuFUGns+kxq2UEXmsjGCQObz uKrAeH9GFEp8BysiZ40SmxxoegENURCDTUi2JTXdXpd0mLLrhNRQab6NOgWCIERYym4D kb/fY5YbMnok8bUdGysgDSX8Q1eZcdqFEKKN+DRA/ZE0LPRmLOuxEjpEo2QhLKw00JkJ 0t693Cw8SQi4YWRodZGw4HytDKIdergYdFnLaGyztoUUX0+vjzmAFJrymTG/sa7iVHKc 2Q== Received: from rn-mailsvcp-mta-lapp01.rno.apple.com (rn-mailsvcp-mta-lapp01.rno.apple.com [10.225.203.149]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 35yc17kkm8-10 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 13 Jan 2021 19:56:08 -0800 Received: from rn-mailsvcp-mmp-lapp02.rno.apple.com (rn-mailsvcp-mmp-lapp02.rno.apple.com [17.179.253.15]) by rn-mailsvcp-mta-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.7.20201203 64bit (built Dec 3 2020)) with ESMTPS id <0QMW011BDO9J37H0@rn-mailsvcp-mta-lapp01.rno.apple.com>; Wed, 13 Jan 2021 19:56:07 -0800 (PST) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp02.rno.apple.com by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.6.20200729 64bit (built Jul 29 2020)) id <0QMW01100O5K3W00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Wed, 13 Jan 2021 19:56:07 -0800 (PST) X-Va-A: X-Va-T-CD: f900b3001c7ef03eb53e4f1f41858654 X-Va-E-CD: 058bb74c67445ec793dcbcd532608805 X-Va-R-CD: 0718ef0ebf1ba6a3a5201f42a655b24e X-Va-CD: 0 X-Va-ID: d40afe47-051e-4c91-b825-5eef9fb42c96 X-V-A: X-V-T-CD: f900b3001c7ef03eb53e4f1f41858654 X-V-E-CD: 058bb74c67445ec793dcbcd532608805 X-V-R-CD: 0718ef0ebf1ba6a3a5201f42a655b24e X-V-CD: 0 X-V-ID: 7b18379a-3973-4f8d-9555-27a329af3c45 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343,18.0.737 definitions=2021-01-14_01:2021-01-13,2021-01-14 signatures=0 Received: from [17.235.2.22] (unknown [17.235.2.22]) by rn-mailsvcp-mmp-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.6.20200729 64bit (built Jul 29 2020)) with ESMTPSA id <0QMW00OC4O9HHC00@rn-mailsvcp-mmp-lapp02.rno.apple.com>; Wed, 13 Jan 2021 19:56:07 -0800 (PST) From: "Andrew Fish" Message-id: MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [edk2-devel] Is CoreValidateHandle() safe? Date: Wed, 13 Jan 2021 19:56:05 -0800 In-reply-to: <16596A642FC06686.22089@groups.io> Cc: Mike Kinney To: edk2-devel-groups-io , Andrew Fish References: <3BAF6AE3-F864-453D-8442-779442E9E342@apple.com> <16596A642FC06686.22089@groups.io> X-Mailer: Apple Mail (2.3654.20.0.2.1) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343,18.0.737 definitions=2021-01-14_01:2021-01-13,2021-01-14 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_7BB7CCBD-8775-4D49-A23B-DBC4AC569264" --Apple-Mail=_7BB7CCBD-8775-4D49-A23B-DBC4AC569264 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Mike, I filed: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3166 Thanks, Andrew Fish > On Jan 11, 2021, at 11:08 PM, Andrew Fish via groups.io wrote: >=20 >=20 >=20 >> On Jan 11, 2021, at 5:51 PM, Michael D Kinney > wrote: >>=20 >> Hi Andrew, >> >> Isn=E2=80=99t the more typical condition for running into this CR ASSER= T is that the calling code cached a copy of a handle that the calling code = had freed before the call was made? >> >=20 > Mike, >=20 > Well in my case the UserHandle is coming from a gBS->HandleProtocol() ca= ll in the BDS that is looping on PCI devices. So things seem like they shou= ld be stable, but there is a high end TB Monitor attached so maybe somethin= g glitched on ThuderBolt?=20 >=20 > If I=E2=80=99m not mistaken this code is walking the entire Handle datab= ase at the time it is called to find a match. All passing in a bogus handle= would do is force you to walk the entire handle list?=20 >=20 > This failed on a RELEASE ROM so I=E2=80=99ve got limited logging and LTO= is on so hard to extract all the details from the crash frame.=20 >=20 >> I agree it look like there may be a tiny window for a timer event. But= even if we move the lock before CoreValidateHandle(), the timer could be s= ignaled >> right before the call was made. Once again, seems like the design of t= he calling code and its events need to make sure a freed handle is never pa= ssed in. >> >=20 > Well as I mentioned it looked to me like this code is mostly just walkin= g the entire gHandleList looking for a match? So passing in a stale handle = would just force a walk of the entire list. I don=E2=80=99t see much differ= ence between that and looking for the last handle?=20 >=20 > Actually I=E2=80=99ve got an lldb command to dump the handle database. I= can see the UserHandle in the database when I connect via the debugger so = gHandleList looks valid at the time of the crash. This is a little more evi= dence I=E2=80=99m hitting a stale Link pointer.=20 >=20 > Thanks, >=20 > Andrew Fish >=20 >> Mike >> >> From: devel@edk2.groups.io > On Behalf Of Andrew Fish via groups.= io >> Sent: Monday, January 11, 2021 4:04 PM >> To: edk2-devel-groups-io > >> Subject: [edk2-devel] Is CoreValidateHandle() safe? >> >> I just hit the CR ASSERT [1] in CoreValidateHandle(). It looks like the= IHANDLE was a use after free as it was a Pool buffer that was to small to = be an IHANDLE and it did not have a valid handle.=20 >> >> I=E2=80=99m trying to understand why it is safe to walk the gHandleList= without a lock? Seems like a local could cache a pointer and an event coul= d remove a handle and Link would point to a stale handle? >> >> Kind of feels like I=E2=80=99m missing something? >> >> [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe= /Hand/Handle.c#L64 >> EFI_STATUS >> CoreValidateHandle ( >> IN EFI_HANDLE UserHandle >> ) >> { >> IHANDLE *Handle; >> LIST_ENTRY *Link; >> >> if (UserHandle =3D=3D NULL) { >> return EFI_INVALID_PARAMETER; >> } >> >> for (Link =3D gHandleList.BackLink; Link !=3D &gHandleList; Link =3D L= ink->BackLink) { >> Handle =3D CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE); >> if (Handle =3D=3D (IHANDLE *) UserHandle) { >> return EFI_SUCCESS; >> } >> } >> >> return EFI_INVALID_PARAMETER; >> } >> >> Thanks, >> >> Andrew Fish >=20 >=20 --Apple-Mail=_7BB7CCBD-8775-4D49-A23B-DBC4AC569264 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Mike,

I filed: https://bugzilla.tianocore.org/= show_bug.cgi?id=3D3166

Thanks,

Andrew Fish

On Jan 11, 2021, at 11:08 PM, Andrew Fish via groups.io <afish=3Dapple.com@groups.io> wr= ote:



On Jan 11, 2021, at 5:51 PM, Michael D Kinney <= michael.d.kinney@i= ntel.com> wrote:

Hi Andrew,
 
Isn=E2=80=99t the more typical condition for running into this CR ASSERT= is that the calling code cached a copy of a handle that the calling code h= ad freed before the call was made?
 <= /div>

Mike,

Wel= l in my case the UserHandle is coming from a gBS->HandleProtocol() call = in the BDS that is looping on PCI devices. So things seem like they should = be stable, but there is a high end TB Monitor attached so maybe something g= litched on ThuderBolt? 

If I=E2=80=99m not mistaken this code is walking the entire Han= dle database at the time it is called to find a match. All passing in a bog= us handle would do is force you to walk the entire handle list? 
=

This failed on a RELEA= SE ROM so I=E2=80=99ve got limited logging and LTO is on so hard to extract= all the details from the crash frame. 

I agree it look like there may be a tiny window for a timer event= .  = But even if we move the lock before CoreValidateHandle(), the t= imer could be signaled
right before the call was made.=   Once again,= seems like the design of the calling code and its events need to make sure= a freed handle is never passed in.
 =

Well as I mentioned it looked to me like this code is mostly just = walking the entire gHandleList looking for a match? So passing in a stale h= andle would just force a walk of the entire list. I don=E2=80=99t see much = difference between that and looking for the last handle? 

Actually I=E2=80=99ve got an = lldb command to dump the handle database. I can see the UserHandle in the d= atabase when I connect via the debugger so gHandleList looks valid at = the time of the crash. This is a little more evidence I=E2=80=99m hitting a= stale Link pointer. 

Thanks,

Andrew Fish

Mike
 
From: devel@edk2.groups.io <devel@edk2.groups= .io> On Behalf Of Andrew = Fish via groups.io
Sent: Monday, January 11, 202= 1 4:04 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Subject: [edk2= -devel] Is CoreValidateHandle() safe?
 
I just hit the CR = ASSERT [1] in CoreValidateHandle(). It looks like the IHANDLE was a us= e after free as it was a Pool buffer that was to small to be an IHANDLE and= it did not have a valid handle. 
 
I=E2=80=99m trying to understand why it is safe to= walk the gHandleList without a lock? Seems like a local could cache a poin= ter and an event could remove a handle and Link would point to a stale hand= le?
 
Kind of feel= s like I=E2=80=99m missing something?
 
[1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/= Core/Dxe/Hand/Handle.c#L64
= =
EFI_STATUS
CoreV= alidateHandle (
=
  IN  EFI_HANDLE       &n= bsp;        UserHandle
  )
{
  IHANDLE            <= span class=3D"Apple-converted-space"> *Handle;
=
  LIST= _ENTRY        &nbs= p; *Link;
 
=
 if (UserHandle =3D=3D NULL) {
    return EFI_INVALID_PARAMETER;
  = }
 =
 for (Link =3D gHan= dleList.BackLink; Link !=3D &gHandleList; Link =3D Link->BackLink) {
=
    Handle =3D CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATU= RE);
  <= span class=3D"Apple-converted-space"> &= nbsp;if (Handle =3D=3D (IHANDLE *) UserHandle) {
      <= /span>return EFI_SUCCESS;
    = }
&nb= sp; }
 
<= span class=3D""> return EFI_INVALID_PARAMETER;
}
 
Thanks= ,
 
=
Andrew Fish



--Apple-Mail=_7BB7CCBD-8775-4D49-A23B-DBC4AC569264--