Re: [edk2-devel] Is CoreValidateHandle() safe?

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 3664 bytes --]

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 <afish=apple.com@groups.io> wrote:
> 
> 
> 
>> On Jan 11, 2021, at 5:51 PM, Michael D Kinney <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Andrew Fish via groups.io <http://groups.io/>
>> Sent: Monday, January 11, 2021 4:04 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io <mailto: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 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 <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
> 
> 


[-- Attachment #2: Type: text/html, Size: 29077 bytes --]