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.z...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.w...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
