From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D32B32117D741 for ; Tue, 23 Oct 2018 11:26:17 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3973E3002F99; Tue, 23 Oct 2018 18:26:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-214.rdu2.redhat.com [10.10.120.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id 78317601A7; Tue, 23 Oct 2018 18:26:14 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Michael D Kinney , Ruiyu Ni , Jiewen Yao , Star Zeng References: <20181023145331.5768-1-jian.j.wang@intel.com> <20181023145331.5768-4-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <71e87828-fcc6-ba1e-cac0-31e4050f795c@redhat.com> Date: Tue, 23 Oct 2018 20:26:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181023145331.5768-4-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Tue, 23 Oct 2018 18:26:16 +0000 (UTC) Subject: Re: [PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Oct 2018 18:26:18 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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