[edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
Laszlo Ersek
lersek at redhat.com
Tue Nov 3 14:01:21 UTC 2020
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 (#66927): https://edk2.groups.io/g/devel/message/66927
Mute This Topic: https://groups.io/mt/77868635/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