Is CoreValidateHandle() safe?

Is CoreValidateHandle() safe?

[Andrew Fish]

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

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

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

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

[Michael D Kinney]

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

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?

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.

Mike

From: devel@edk2.groups.io <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 <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
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: 60146 bytes --]

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

[Andrew Fish]

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



> On Jan 11, 2021, at 5:51 PM, Michael D Kinney <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 <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 <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: 27475 bytes --]

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

[Andrew Fish]

[-- 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 --]