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