* 回复: [edk2-devel] [PATCH v2] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-10-27 15:52 ` [PATCH v2] MdeModulePkg/Gcd: Check memory allocation when initializing memory Jeff Brasen
@ 2020-10-28 6:06 ` gaoliming
0 siblings, 0 replies; 2+ messages in thread
From: gaoliming @ 2020-10-28 6:06 UTC (permalink / raw)
To: devel, jbrasen; +Cc: dandan.bi, lersek
Jeff:
The logic is correct. It handles the memory allocated by creating memory
allocation hob instead of allocating page service.
I add minor comment on below.
> -----邮件原件-----
> 发件人: bounce+27952+66661+4905953+8761045@groups.io
> <bounce+27952+66661+4905953+8761045@groups.io> 代表 Jeff Brasen
> 发送时间: 2020年10月27日 23:53
> 收件人: devel@edk2.groups.io
> 抄送: dandan.bi@intel.com; gaoliming@byosoft.com.cn; lersek@redhat.com;
> Jeff Brasen <jbrasen@nvidia.com>
> 主题: [edk2-devel] [PATCH v2] MdeModulePkg/Gcd: Check memory
> allocation when initializing memory
>
> 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@nvidia.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 60
> +++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 2d8c076f7113..4a22ee96b758 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -2097,6 +2097,62 @@ 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;
> +
> + TopAddress = *BaseAddress + *Length;
> + while (MemoryHob != NULL) {
> + EFI_PHYSICAL_ADDRESS AllocatedTop;
> +
Please move local variable declaration in the begin of the function.
> + AllocatedTop = MemoryHob->AllocDescriptor.MemoryBaseAddress +
> MemoryHob->AllocDescriptor.MemoryLength;
> +
> + if ((MemoryHob->AllocDescriptor.MemoryBaseAddress >=
> *BaseAddress) &&
> + (AllocatedTop <= TopAddress)) {
> + EFI_PHYSICAL_ADDRESS LowerBase;
> + UINT64 LowerSize;
> + EFI_PHYSICAL_ADDRESS UpperBase;
> + UINT64 UpperSize;
> +
Please move local variable declaration in the begin of the function.
With this change, Liming Gao <gaoliming@byosoft.com.cn>
Thanks
Liming
> + 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 +2291,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 +2299,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 +2307,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 +2371,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;
> }
> --
> 2.25.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread