Re: [PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump

[Laszlo Ersek <lersek@redhat.com>]

On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Update implementation of CoreGetMemorySpaceMap() and
>>    CoreGetIoSpaceMap() to avoid lock failure. Drop the code to
>>    detect debug print level used to achieve the same effect.
>
> This issue is hidden in current code but exposed by introduction
> of freed-memory guard feature due to the fact that the feature
> will turn all pool allocation to page allocation.
>
> The solution is move the memory allocation in CoreGetMemorySpaceMap()
> and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
>
>    CoreDumpGcdMemorySpaceMap()
> => CoreGetMemorySpaceMap()
> => CoreAcquireGcdMemoryLock () *
>    AllocatePool()
> => InternalAllocatePool()
> => CoreAllocatePool()
> => CoreAllocatePoolI()
> => CoreAllocatePoolPagesI()
> => CoreAllocatePoolPages()
> => FindFreePages()
> => PromoteMemoryResource()
> => CoreAcquireGcdMemoryLock()  **
>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 140 ++++++++++++++++++++++++----------------
>  1 file changed, 86 insertions(+), 54 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index d9c65a8045..133c3dcd87 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
>    OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>    )
>  {
> -  EFI_STATUS                       Status;
>    LIST_ENTRY                       *Link;
>    EFI_GCD_MAP_ENTRY                *Entry;
>    EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
> +  UINTN                            Number;
>
>    //
>    // Make sure parameters are valid
> @@ -1706,38 +1706,54 @@ CoreGetMemorySpaceMap (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireGcdMemoryLock ();
> -
>    //
>    // Count the number of descriptors
>    //
> -  *NumberOfDescriptors = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +  Number = 0;
> +  *NumberOfDescriptors = 0;
> +  *MemorySpaceMap = NULL;
> +  while (TRUE) {
> +    //
> +    // Allocate the MemorySpaceMap
> +    //
> +    if (Number != 0) {
> +      if (*MemorySpaceMap != NULL) {
> +        FreePool (*MemorySpaceMap);
> +      }
>
> -  //
> -  // Allocate the MemorySpaceMap
> -  //
> -  *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
> -  if (*MemorySpaceMap == NULL) {
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto Done;
> -  }
> +      *MemorySpaceMap = AllocatePool (Number * sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));

(1) Side comment: you dropped the space character after "sizeof".

> +      if (*MemorySpaceMap == NULL) {
> +        return EFI_OUT_OF_RESOURCES;
> +      }
>
> -  //
> -  // Fill in the MemorySpaceMap
> -  //
> -  Descriptor = *MemorySpaceMap;
> -  Link = mGcdMemorySpaceMap.ForwardLink;
> -  while (Link != &mGcdMemorySpaceMap) {
> -    Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> -    BuildMemoryDescriptor (Descriptor, Entry);
> -    Descriptor++;
> -    Link = Link->ForwardLink;
> +      *NumberOfDescriptors = Number;
> +    }
> +
> +    CoreAcquireGcdMemoryLock ();
> +
> +    Number = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
> +    if (Number == *NumberOfDescriptors) {
> +      //
> +      // Fill in the MemorySpaceMap
> +      //
> +      Descriptor = *MemorySpaceMap;
> +      Link = mGcdMemorySpaceMap.ForwardLink;
> +      while (Link != &mGcdMemorySpaceMap && Number != 0) {
> +        Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> +        BuildMemoryDescriptor (Descriptor, Entry);
> +        Descriptor++;
> +        Number--;
> +        Link = Link->ForwardLink;
> +      }
> +
> +      CoreReleaseGcdMemoryLock ();
> +      break;
> +    }
> +
> +    CoreReleaseGcdMemoryLock ();
>    }
> -  Status = EFI_SUCCESS;
>
> -Done:
> -  CoreReleaseGcdMemoryLock ();
> -  return Status;
> +  return EFI_SUCCESS;
>  }

I think this loop can be improved. Here's the facts that I don't like
about it:

(2) The "Number" variable name is bad. It should be "DescriptorCount".

(3) The ways the outer "if"s are formulated in the loop body are hard to
    read. In particular, I think what bothers me the most is that the
    loop body starts with the memory allocation, and not with the
    CoreCountGcdMapEntry() call. I think we can do better.

(4) We have one location in the code that acquires the lock, but two
    locations that release it. While it is technically correct, it's
    hard to read as well.

(5) Decreasing "Number" in the inner "while" loop, and checking it in
    the inner loop condition, seems strange. The GCD memory space map
    will have at least one entry at all times (minimally there is a
    NonExistent entry that covers the entire address space, according to
    the address width from the CPU HOB). In addition, I don't see when
    it could validly occur that Number doesn't match the length of the
    list.

How about the following instead (I'm quoting the full function, untested
/ uncompiled):

> EFI_STATUS
> EFIAPI
> CoreGetMemorySpaceMap (
>   OUT UINTN                            *NumberOfDescriptors,
>   OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR  **MemorySpaceMap
>   )
> {
>   LIST_ENTRY                       *Link;
>   EFI_GCD_MAP_ENTRY                *Entry;
>   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Descriptor;
>   UINTN                            DescriptorCount;
>
>   //
>   // Make sure parameters are valid
>   //
>   if (NumberOfDescriptors == NULL) {
>     return EFI_INVALID_PARAMETER;
>   }
>   if (MemorySpaceMap == NULL) {
>     return EFI_INVALID_PARAMETER;
>   }
>
>   //
>   // No candidate map allocated yet.
>   //
>   *NumberOfDescriptors = 0;
>   *MemorySpaceMap = NULL;
>
>   //
>   // Take the lock, for entering the loop with the lock held.
>   //
>   CoreAcquireGcdMemoryLock ();
>   while (TRUE) {
>     //
>     // Count the descriptors.
>     //
>     DescriptorCount = CoreCountGcdMapEntry (&mGcdMemorySpaceMap);
>
>     if (DescriptorCount == *NumberOfDescriptors) {
>       //
>       // Fill in the MemorySpaceMap
>       //
>       Descriptor = *MemorySpaceMap;
>       Link = mGcdMemorySpaceMap.ForwardLink;
>       while (Link != &mGcdMemorySpaceMap) {
>         Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
>         BuildMemoryDescriptor (Descriptor, Entry);
>         Descriptor++;
>         Link = Link->ForwardLink;
>       }
>
>       //
>       // We're done; exit the loop with the lock held.
>       //
>       break;
>     }
>
>     CoreReleaseGcdMemoryLock ();
>
>     //
>     // Free previous allocation, with the lock released.
>     //
>     if (*MemorySpaceMap != NULL) {
>       FreePool (*MemorySpaceMap);
>     }
>
>     //
>     // Allocate the MemorySpaceMap, with the lock released.
>     //
>     *MemorySpaceMap = AllocatePool (
>                         (DescriptorCount *
>                          sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR))
>                         );
>     if (*MemorySpaceMap == NULL) {
>       *NumberOfDescriptors = 0;
>       return EFI_OUT_OF_RESOURCES;
>     }
>     *NumberOfDescriptors = DescriptorCount;
>
>     //
>     // Re-acquire the lock, for the next iteration.
>     //
>     CoreAcquireGcdMemoryLock ();
>   }
>
>   //
>   // We exited the loop with the lock held, release it.
>   //
>   CoreReleaseGcdMemoryLock ();
>
>   return EFI_SUCCESS;
> }
>

I don't insist on this variant, of course, it's just an idea to discuss!
If others find your variant easier to read, I'm fine with it as well.
(Still, I think points (1), (2) and (5) should be fixed.)


(6) I've snipped the CoreGetIoSpaceMap() changes because, AFAICS, they
mirror the CoreGetMemorySpaceMap() changes. However: do we *really* need
to update CoreGetIoSpaceMap()? Because, allocating pool memory, even if
it ends up allocating page memory, should be independent of the *IO Port
space* in GCD.

If you fix CoreGetMemorySpaceMap() but don't touch CoreGetIoSpaceMap(),
can you actually trigger a deadlock (with or without enabling the UAF
guard)?

Thanks,
Laszlo