From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by mx.groups.io with SMTP id smtpd.web12.5913.1610435320121244185 for ; Mon, 11 Jan 2021 23:08:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=LjDuVlmo; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.42/8.16.0.42) with SMTP id 10C6imDB025165; Mon, 11 Jan 2021 23:08:39 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=from : content-type : mime-version : subject : date : references : to : in-reply-to : message-id; s=20180706; bh=e10vHDv7QeaCXGUnxD9Ti/7ergvAe5XZND/qiFwGJ8M=; b=LjDuVlmoFJzKe+AqtFRNotpYq86iLkXZauRClNjof/4W4b/1oVCRNC8wI+68K6iRcf21 UrJk4z2HHVziBcXEoPL7OWVGT1apmOVCeURZLHYHNuHuzwJKEwlUosVFgMOHvuKlbmD8 hUcwTQFWvhhz0lwe+3tW+lkY+gjzqpYBeETi2dL3EcNMDkjjiwN2x0e/1gAJtWsSYCGY 7F2vh22Mbzf6BmRJi5KxioTm5q/bEcY6q43BAJAwwZUwzgV+a3fZF2hGMMx1yMhaL+7o tMRY3FqhCA1ps9BxL4opfJvDbBJXK52rBBW6zHYI4wGYl7m2g9aKOLecyh4nb/+FPOQC wA== Received: from rn-mailsvcp-mta-lapp02.rno.apple.com (rn-mailsvcp-mta-lapp02.rno.apple.com [10.225.203.150]) by nwk-aaemail-lapp03.apple.com with ESMTP id 360x55047t-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Mon, 11 Jan 2021 23:08:39 -0800 Received: from rn-mailsvcp-mmp-lapp01.rno.apple.com (rn-mailsvcp-mmp-lapp01.rno.apple.com [17.179.253.14]) by rn-mailsvcp-mta-lapp02.rno.apple.com (Oracle Communications Messaging Server 8.1.0.6.20200729 64bit (built Jul 29 2020)) with ESMTPS id <0QMT00P477UFCOH0@rn-mailsvcp-mta-lapp02.rno.apple.com>; Mon, 11 Jan 2021 23:08:39 -0800 (PST) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp01.rno.apple.com by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.6.20200729 64bit (built Jul 29 2020)) id <0QMT00A007Q6H600@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Mon, 11 Jan 2021 23:08:39 -0800 (PST) X-Va-A: X-Va-T-CD: 70a38c3f5b1d46c4b8dccb3b011be358 X-Va-E-CD: 058bb74c67445ec793dcbcd532608805 X-Va-R-CD: 0718ef0ebf1ba6a3a5201f42a655b24e X-Va-CD: 0 X-Va-ID: 318edaf9-3f43-4ae5-a321-69a74219349a X-V-A: X-V-T-CD: 70a38c3f5b1d46c4b8dccb3b011be358 X-V-E-CD: 058bb74c67445ec793dcbcd532608805 X-V-R-CD: 0718ef0ebf1ba6a3a5201f42a655b24e X-V-CD: 0 X-V-ID: d7adda57-9b2d-483b-ab63-343305e2d37d X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343,18.0.737 definitions=2021-01-12_03:2021-01-11,2021-01-12 signatures=0 Received: from [17.235.18.245] (unknown [17.235.18.245]) by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.6.20200729 64bit (built Jul 29 2020)) with ESMTPSA id <0QMT00UIR7UDA900@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Mon, 11 Jan 2021 23:08:38 -0800 (PST) From: "Andrew Fish" MIME-version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.1\)) Subject: Re: [edk2-devel] Is CoreValidateHandle() safe? Date: Mon, 11 Jan 2021 23:08:37 -0800 References: <3BAF6AE3-F864-453D-8442-779442E9E342@apple.com> To: edk2-devel-groups-io , Mike Kinney In-reply-to: Message-id: 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-12_03:2021-01-11,2021-01-12 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_BB913DC0-F2A1-4110-B888-38A8D69800FF" --Apple-Mail=_BB913DC0-F2A1-4110-B888-38A8D69800FF Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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 ASSERT= is that the calling code cached a copy of a handle that the calling code h= ad freed before the call was made? > Mike, Well 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 = glitched on ThuderBolt?=20 If I=E2=80=99m not mistaken this code is walking the entire Handle databas= e at the time it is called to find a match. All passing in a bogus handle w= ould do is force you to walk the entire handle list?=20 This failed on a RELEASE ROM so I=E2=80=99ve got limited logging and LTO i= s on so hard to extract all the details from the crash frame.=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 si= gnaled > right before the call was made. Once again, seems like the design of th= e calling code and its events need to make sure a freed handle is never pas= sed 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 handle wo= uld just force a walk of the entire list. I don=E2=80=99t see much differen= ce between that and looking for the last handle?=20 Actually I=E2=80=99ve got an lldb command to dump the handle database. I c= an see the UserHandle in the database when I connect via the debugger so gH= andleList looks valid at the time of the crash. This is a little more evide= nce I=E2=80=99m hitting a stale Link pointer.=20 Thanks, Andrew Fish > Mike > > From: devel@edk2.groups.io On Behalf Of Andrew Fi= sh 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 b= e 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 could= 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 Li= nk->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 --Apple-Mail=_BB913DC0-F2A1-4110-B888-38A8D69800FF Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On Jan 11, 2= 021, at 5:51 PM, Michael D Kinney <michael.d.kinney@intel.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 ha= d freed before the call was made?
 

Mike,

Well 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 glitched on ThuderBolt? 

If I=E2=80=99m not mistaken this code is walking the en= tire Handle database 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?&nbs= p;

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

I a= gree 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 signaled
<= span class=3D"">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 ha= ndle is never passed in.
 

Well as I mentioned it l= ooked to me like this code is mostly just walking the entire gHandleList lo= oking 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 difference between that and looki= ng 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 database when I connect via the debugger so gHandle= List 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:&nb= sp;devel@edk2.gro= ups.io <devel@edk= 2.groups.io> On Behalf Of Andrew Fish via groups.io
Sent:&nbs= p;Monday, January 11, 2021 4:04 PM
To: edk2-devel-groups-io &= lt;devel@edk2.groups.io<= /a>>
Subject: [edk2-devel] Is CoreValidateHandle() safe?
 
I just hit the CR ASSERT [1] in CoreValidateHandle(). It lo= oks 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. 
 
&n= bsp;
Kind of feels like I=E2=80=99m missing something?
 
[1] https://github.com/tianocore/edk= 2/blob/master/MdeModulePkg/Core/Dxe/Hand/Handle.c#L64
<= tr style=3D"box-sizing: border-box;" class=3D""><= td width=3D"50" nowrap=3D"" valign=3D"top" id=3D"L73" style=3D"width: 37.5p= t; padding: 0in 7.5pt; box-sizing: border-box; min-width: 50px; color: var(= --color-diff-blob-num-text); cursor: pointer; -webkit-user-select: none;" c= lass=3D""><= tr style=3D"box-sizing: border-box;" class=3D"">=
EFI_STATUS
CoreValidateHandle (=
 &= nbsp;IN  EFI_HANDLE   &nb= sp;            UserHandle
 = ; )
{
  IHANDLE        &= nbsp;    <= /span>*Handle;
  = LIST_ENTRY     &nbs= p;    *Link;
 =
 = if (UserHandle =3D= = =3D NULL) {
&n= bsp;   return EFI_INVALID_PARAMETER;
  }
 
 = for = (Link =3D gHandleList.BackLink; L= ink !=3D &gHandleList; Link =3D Link->= BackLink) {
  &= nbsp; Hand= le =3D CR (Link, IHANDLE, AllHandles,= EFI_HANDLE_SIGNATURE);
    if (Handle =3D=3D (IHANDLE *) Use= rHandle) {
 = ;     return EFI_SUCCESS;
    }
  <= /span>}
 = ;
 return EFI_INV= ALID_PARAMETER;
}
 = ;
Thanks,
 <= /span>
Andrew Fish
=


--Apple-Mail=_BB913DC0-F2A1-4110-B888-38A8D69800FF--