Mike, I filed: https://bugzilla.tianocore.org/show_bug.cgi?id=3166 Thanks, Andrew Fish > On Jan 11, 2021, at 11:08 PM, Andrew Fish via groups.io wrote: > > > >> On Jan 11, 2021, at 5:51 PM, Michael D Kinney > wrote: >> >> Hi Andrew, >> >> Isn’t 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 had 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’m not mistaken this code is walking the entire 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? > > This failed on a RELEASE ROM so I’ve 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 timer 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 handle would just force a walk of the entire list. I don’t see much difference between that and looking for the last handle? > > Actually I’ve 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 evidence I’m hitting a stale Link pointer. > > Thanks, > > Andrew Fish > >> 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. >> >> I’m 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’m 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 == NULL) { >> return EFI_INVALID_PARAMETER; >> } >> >> for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) { >> Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE); >> if (Handle == (IHANDLE *) UserHandle) { >> return EFI_SUCCESS; >> } >> } >> >> return EFI_INVALID_PARAMETER; >> } >> >> Thanks, >> >> Andrew Fish > >