Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

["Ma, Hua" <hua.ma@intel.com>]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Monday, October 11, 2021 7:28 PM
> To: devel@edk2.groups.io; Ma, Hua <hua.ma@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Bi, Dandan <dandan.bi@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock
> when iterating gHandleList
> 
> On 11/10/2021 11:45, Ma, Hua wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680
> >
> > This patch fixes the following issue:
> >
> > The global variable gHandleList is a linked list.
> > This list is locked when a entry is added or removed from the list,
> > but there is no lock when iterating this list in function
> > CoreValidateHandle().
> > It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
> > iterated entry in the list is just removed by other task during iterating.
> > Locking the list when iterating can fix this issue.
> 
> At a first glance, it looks as though if the caller does not already
> hold the lock, then the result from CoreValidateHandle() may be invalid
> by the time that control returns to the caller.
> 
> Under what circumstances is it valid to call CoreValidateHandle() when
> the caller does not _already_ hold the lock (i.e. IsLocked==FALSE)?
> 
> Thanks,
> 
> Michael

Hi Michael,

Thanks for the review,
In current CoreValidateHandle implementation:

  for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
    Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
    if (Handle == (IHANDLE *) UserHandle) {
      return EFI_SUCCESS;
    }
  }

Let's say, if the list have 4 entries, (A->B->C->D), and the caller is trying to validate if entry C is valid,
When caller task just set local variable Link to B, and before it calls CR() macro to validate the signature,
At this point, if other task just deletes other entry, entry B, and reset B's signature. 
Then when back to caller task,  There is a signature mismatch assertion for B.
But the result should be still valid for C if call CoreValidateHandle() again.
The patch is trying to fix this kind of problem.

I feel one valid kind of calling CoreValidateHandle() without holding the lock is:
when the handle is passed in as parameter, and do a parameter checking,
and later, if need, validated the handle again when use the handle.
for example in current code, in AddSortedDriverBindingProtocol, CoreConnectController,
This patch do not change where to add a handle validation.

Thank you,
Ma Hua