From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.4452.1604412092682990297 for ; Tue, 03 Nov 2020 06:01:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dz3CFHtz; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1604412091; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Z2qDFIffx70HbbW5Ky/2w9E1y2b7EzSUOW8/864l1C8=; b=dz3CFHtzXmjKytLL/a3jI9RCS7eX2cLRkNjwMJ90WB80eHIEEB0aeo7Cia62BDkmzVVJp5 U2E3/Cu+s/FOmXWxwOV0LCFrPu5noiGibSygZ1RJfZIVzyDesXZR9xCQqs6T4QPw7p7pS8 hRCW+C8iBSqyVjMemiEffovE7d1W68M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-492-BlbJ8CvkPGiEXsTWSRb8Gg-1; Tue, 03 Nov 2020 09:01:26 -0500 X-MC-Unique: BlbJ8CvkPGiEXsTWSRb8Gg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F2B758015B1; Tue, 3 Nov 2020 14:01:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-74.ams2.redhat.com [10.36.115.74]) by smtp.corp.redhat.com (Postfix) with ESMTP id 869B56EF73; Tue, 3 Nov 2020 14:01:22 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory To: devel@edk2.groups.io, jbrasen@nvidia.com Cc: dandan.bi@intel.com, gaoliming@byosoft.com.cn, "Ard Biesheuvel (ARM address)" , "Leif Lindholm (Nuvia address)" References: From: "Laszlo Ersek" Message-ID: <11029d65-f4be-b563-ef8d-0f616fccf2f2@redhat.com> Date: Tue, 3 Nov 2020 15:01:21 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/28/20 18:35, Jeff Brasen wrote: > CoreInitializeMemoryServices was not checking for any existing memory > allocation created in the HOB producer phase. If there are memory > allocations outside of the region covered by the HOB List then Gcd could > select that region for memory which can result in the memory allocation > to not be handled and memory overwrites. > > Signed-off-by: Jeff Brasen > --- > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 58 +++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > index 2d8c076f7113..51b082b7e7eb 100644 > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > @@ -2097,6 +2097,60 @@ CalculateTotalMemoryBinSizeNeeded ( > return TotalSize; > } > > +/** > + Find the largest region in the specified region that is not covered by an existing memory allocation > + > + @param BaseAddress On input start of the region to check. > + On output start of the largest free region. > + @param Length On input size of region to check. > + On output size of the largest free region. > + @param MemoryHob Hob pointer for the first memory allocation pointer to check > +**/ > +VOID > +FindLargestFreeRegion ( > + IN OUT EFI_PHYSICAL_ADDRESS *BaseAddress, > + IN OUT UINT64 *Length, > + IN EFI_HOB_MEMORY_ALLOCATION *MemoryHob > + ) > +{ > + EFI_PHYSICAL_ADDRESS TopAddress; > + EFI_PHYSICAL_ADDRESS AllocatedTop; > + EFI_PHYSICAL_ADDRESS LowerBase; > + UINT64 LowerSize; > + EFI_PHYSICAL_ADDRESS UpperBase; > + UINT64 UpperSize; > + > + TopAddress = *BaseAddress + *Length; > + while (MemoryHob != NULL) { > + AllocatedTop = MemoryHob->AllocDescriptor.MemoryBaseAddress + MemoryHob->AllocDescriptor.MemoryLength; > + > + if ((MemoryHob->AllocDescriptor.MemoryBaseAddress >= *BaseAddress) && > + (AllocatedTop <= TopAddress)) { > + LowerBase = *BaseAddress; > + LowerSize = MemoryHob->AllocDescriptor.MemoryBaseAddress - *BaseAddress; > + UpperBase = AllocatedTop; > + UpperSize = TopAddress - AllocatedTop; > + > + if (LowerSize != 0) { > + FindLargestFreeRegion (&LowerBase, &LowerSize, (EFI_HOB_MEMORY_ALLOCATION *) GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob))); > + } > + if (UpperSize != 0) { > + FindLargestFreeRegion (&UpperBase, &UpperSize, (EFI_HOB_MEMORY_ALLOCATION *) GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob))); > + } > + > + if (UpperSize >= LowerSize) { > + *Length = UpperSize; > + *BaseAddress = UpperBase; > + } else { > + *Length = LowerSize; > + *BaseAddress = LowerBase; > + } > + return; > + } > + MemoryHob = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, GET_NEXT_HOB (MemoryHob)); > + } > +} > + > /** > External function. Initializes memory services based on the memory > descriptor HOBs. This function is responsible for priming the memory > @@ -2235,6 +2289,7 @@ CoreInitializeMemoryServices ( > Attributes = PhitResourceHob->ResourceAttribute; > BaseAddress = PageAlignAddress (PhitHob->EfiMemoryTop); > Length = PageAlignLength (ResourceHob->PhysicalStart + ResourceHob->ResourceLength - BaseAddress); > + FindLargestFreeRegion (&BaseAddress, &Length, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION)); > if (Length < MinimalMemorySizeNeeded) { > // > // If that range is not large enough to intialize the DXE Core, then > @@ -2242,6 +2297,7 @@ CoreInitializeMemoryServices ( > // > BaseAddress = PageAlignAddress (PhitHob->EfiFreeMemoryBottom); > Length = PageAlignLength (PhitHob->EfiFreeMemoryTop - BaseAddress); > + //This region is required to have no memory allocation inside it, skip check for entries in HOB List > if (Length < MinimalMemorySizeNeeded) { > // > // If that range is not large enough to intialize the DXE Core, then > @@ -2249,6 +2305,7 @@ CoreInitializeMemoryServices ( > // > BaseAddress = PageAlignAddress (ResourceHob->PhysicalStart); > Length = PageAlignLength ((UINT64)((UINTN)*HobStart - BaseAddress)); > + FindLargestFreeRegion (&BaseAddress, &Length, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION)); > } > } > break; > @@ -2312,6 +2369,7 @@ CoreInitializeMemoryServices ( > // > TestedMemoryBaseAddress = PageAlignAddress (ResourceHob->PhysicalStart); > TestedMemoryLength = PageAlignLength (ResourceHob->PhysicalStart + ResourceHob->ResourceLength - TestedMemoryBaseAddress); > + FindLargestFreeRegion (&TestedMemoryBaseAddress, &TestedMemoryLength, (EFI_HOB_MEMORY_ALLOCATION *)GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION)); > if (TestedMemoryLength < MinimalMemorySizeNeeded) { > continue; > } > I applied this on top of commit 0166dad49698 ("BaseTools: Update the FV Space Information to display decimal and Hex", 2020-11-03). * In my regression testing with ArmVirtQemu (AARCH64 -- 1 scenario), the only difference seems to be the following, in the firmware log: > -HOBLIST address in DXE = 0x13B641018 > +HOBLIST address in DXE = 0x13B621018 Plus, a whole lot of subsequent addresses are consistently decreased by 0x2_0000. I could see or experience no other differences. * In my regression testing with OVMF, with S3 disabled (IA32 (q35), X64 (i440fx), IA32X64 (q35) -- 3 scenarios): I saw some addresses in the log grow by 0x280 (IA32) or 0x200 (X64) or 0x240 (IA32X64). In the IA32 and X64 cases, these offsets were consistent; in the IA32X64 case, I saw some other (less uniform) address shifts too (for example growth by 0x5_7E80, or decrease by 0x180). * In my regression testing with OVMF, with S3 enabled (IA32 (q35), X64 (i440fx), IA32X64 (q35) -- 3 scenarios): Regarding the "cold boot" related parts of the logs, the same applies as above, except: - the X64 address shifts were less uniform than in the "S3 disabled" case, - the IA32X64 address shifts were more uniform than in the "S3 disabled" case. Regarding the "S3 resume" related parts of the logs, address differences did not affect them at all, for all of IA32, X64, and IA32X64. * In the 3+3=6 OVMF scenarios above, the patch did not change the BaseAddress / Length / MinimalMemorySizeNeeded INFO message printed by CoreInitializeMemoryServices(): - S3 disabled: - IA32: BaseAddress - 0x83423000 Length - 0x3EEA000 MinimalMemorySizeNeeded - 0x320000 - X64: BaseAddress - 0xBBFE2000 Length - 0x3C1E000 MinimalMemorySizeNeeded - 0x320000 - IA32X64: BaseAddress - 0x7AFE1000 Length - 0x3C1E000 MinimalMemorySizeNeeded - 0x4E5000 - S3 enabled: - IA32: BaseAddress - 0x83383000 Length - 0x3EEA000 MinimalMemorySizeNeeded - 0x320000 - X64: BaseAddress - 0xBBF42000 Length - 0x3ABE000 MinimalMemorySizeNeeded - 0x320000 - IA32X64: BaseAddress - 0x7AF21000 Length - 0x3CDE000 MinimalMemorySizeNeeded - 0x4E5000 * In the ArmVirtQemu scenario, I couldn't compare the the "BaseAddress / Length / MinimalMemorySizeNeeded" INFO messages from before/after the patch, because the firmware logs don't seem to contain this message at all. This is despite PcdDebugPrintErrorLevel having DEBUG_INFO (0x00000040) set. ... Oh. This logging issue is similar to the one fixed for OVMF in commit 91a5b1365075 ("OvmfPkg/PlatformDebugLibIoPort: fix port detection for use in the DXE Core", 2018-08-06). Namely, CoreInitializeMemoryServices() calls DEBUG() before the DXE Core calls library constructors explicitly -- so those DEBUGs are issued against an un-constructed DebugLib, and also against a -- possibly underlying -- unconstructed SerialPortLib. I'll propose a very simple patch for that issue soon. For now, I've used said patch locally, for capturing BaseAddress and Length under ArmVirtQemu as well, before and after Jeff's patch. So I could do a comparison even under ArmVirtQemu. Result: > -DxeMain: MemoryBaseAddress=0x40000000 MemoryLength=0xFC020000 > +DxeMain: MemoryBaseAddress=0x40000000 MemoryLength=0xFC000000 So this patch makes an actual difference for ArmVirtQemu! Where does the difference come from? Let's check the log for 0x40000000+0xFC000000 = 0x13C000000: > QemuVirtMemInfoPeiLibConstructor: System RAM @ 0x40000000 - 0x13FFFFFFF > [...] > PeiInstallPeiMemory MemoryBegin 0x13C000000, MemoryLength 0x4000000 > [...] > Old Stack size 8160, New stack size 131072 > Stack Hob: BaseAddress=0x13C000000 Length=0x20000 > [...] > -DxeMain: MemoryBaseAddress=0x40000000 MemoryLength=0xFC020000 > +DxeMain: MemoryBaseAddress=0x40000000 MemoryLength=0xFC000000 > [...] > -HOBLIST address in DXE = 0x13B641018 > +HOBLIST address in DXE = 0x13B621018 > [...] > Memory Allocation 0x00000004 0x13C000000 - 0x13C01FFFF The above happens when the permanent PEI RAM is installed by the platform, and the PEI Core migrates temp RAM to permanent RAM. The CPU stack on the permanent PEI RAM is described by the stack HOB, which is a special EfiBootServicesData memory allocation HOB. It's created by the PEI Core (PeiDispatcher --> PeiCheckAndSwitchStack --> BuildStackHob). The HOB is specified in PI v1.7, Volume 3, section "5.4.2 Boot-Strap Processor (BSP) Stack Memory Allocation HOB". Without Jeff's patch, the DXE core thinks that the area is free memory (initially), and then the memalloc (stack) HOB is processed later. With Jeff's patch, the memalloc (stack) HOB is immediately excluded from DXE memory. In this particular case, I'm not convinced that the pre-patch behavior is unsafe or wrong. But the post-patch behavior also seems to be OK. Regression-tested-by: Laszlo Ersek Thanks Laszlo