回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
gaoliming
gaoliming at byosoft.com.cn
Thu Nov 5 03:10:36 UTC 2020
Laszlo:
Thanks for the full regression test. I create PR for this patch. https://github.com/tianocore/edk2/pull/1085
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+66927+4905953+8761045 at groups.io
> <bounce+27952+66927+4905953+8761045 at groups.io> 代表 Laszlo Ersek
> 发送时间: 2020年11月3日 22:01
> 收件人: devel at edk2.groups.io; jbrasen at nvidia.com
> 抄送: dandan.bi at intel.com; gaoliming at byosoft.com.cn; Ard Biesheuvel
> (ARM address) <ard.biesheuvel at arm.com>; Leif Lindholm (Nuvia address)
> <leif at nuviainc.com>
> 主题: Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory
> allocation when initializing memory
>
> 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 <jbrasen at nvidia.com>
> > ---
> > 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 <lersek at redhat.com>
>
> Thanks
> Laszlo
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67023): https://edk2.groups.io/g/devel/message/67023
Mute This Topic: https://groups.io/mt/78044283/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list