* [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
@ 2020-10-28 17:35 Jeff Brasen
2020-10-30 0:59 ` 回复: [edk2-devel] " gaoliming
2020-11-03 14:01 ` Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Brasen @ 2020-10-28 17:35 UTC (permalink / raw)
To: devel; +Cc: dandan.bi, gaoliming, lersek, Jeff Brasen
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 | 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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-10-28 17:35 [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory Jeff Brasen
@ 2020-10-30 0:59 ` gaoliming
2020-11-02 17:29 ` Laszlo Ersek
2020-11-03 14:01 ` Laszlo Ersek
1 sibling, 1 reply; 6+ messages in thread
From: gaoliming @ 2020-10-30 0:59 UTC (permalink / raw)
To: devel, jbrasen; +Cc: dandan.bi, lersek
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
If no other comment, I will merge this patch early of next week.
> -----邮件原件-----
> 发件人: bounce+27952+66698+4905953+8761045@groups.io
> <bounce+27952+66698+4905953+8761045@groups.io> 代表 Jeff Brasen
> 发送时间: 2020年10月29日 1:35
> 收件人: devel@edk2.groups.io
> 抄送: dandan.bi@intel.com; gaoliming@byosoft.com.cn; lersek@redhat.com;
> Jeff Brasen <jbrasen@nvidia.com>
> 主题: [edk2-devel] [PATCH v3 1/1] 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 | 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;
> }
> --
> 2.25.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-10-30 0:59 ` 回复: [edk2-devel] " gaoliming
@ 2020-11-02 17:29 ` Laszlo Ersek
2020-11-03 0:56 ` 回复: " gaoliming
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2020-11-02 17:29 UTC (permalink / raw)
To: gaoliming, devel, jbrasen; +Cc: dandan.bi
On 10/30/20 01:59, gaoliming wrote:
> Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
>
> If no other comment, I will merge this patch early of next week.
I'd like to regression-test this with OVMF and ArmVirtQemu, but I need a
bit more time -- I just returned to work, and my email backlog is crushing.
Thanks for your patience.
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* 回复: 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-11-02 17:29 ` Laszlo Ersek
@ 2020-11-03 0:56 ` gaoliming
0 siblings, 0 replies; 6+ messages in thread
From: gaoliming @ 2020-11-03 0:56 UTC (permalink / raw)
To: devel, lersek, jbrasen; +Cc: dandan.bi
Laszlo:
OK. I will wait for your test result.
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+66870+4905953+8761045@groups.io
> <bounce+27952+66870+4905953+8761045@groups.io> 代表 Laszlo Ersek
> 发送时间: 2020年11月3日 1:30
> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
> jbrasen@nvidia.com
> 抄送: dandan.bi@intel.com
> 主题: Re: 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check
> memory allocation when initializing memory
>
> On 10/30/20 01:59, gaoliming wrote:
> > Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> >
> > If no other comment, I will merge this patch early of next week.
>
> I'd like to regression-test this with OVMF and ArmVirtQemu, but I need a
> bit more time -- I just returned to work, and my email backlog is
crushing.
>
> Thanks for your patience.
>
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-10-28 17:35 [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory Jeff Brasen
2020-10-30 0:59 ` 回复: [edk2-devel] " gaoliming
@ 2020-11-03 14:01 ` Laszlo Ersek
2020-11-05 3:10 ` 回复: " gaoliming
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2020-11-03 14:01 UTC (permalink / raw)
To: devel, jbrasen
Cc: dandan.bi, gaoliming, Ard Biesheuvel (ARM address),
Leif Lindholm (Nuvia address)
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@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@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory
2020-11-03 14:01 ` Laszlo Ersek
@ 2020-11-05 3:10 ` gaoliming
0 siblings, 0 replies; 6+ messages in thread
From: gaoliming @ 2020-11-05 3:10 UTC (permalink / raw)
To: devel, lersek, jbrasen
Cc: dandan.bi, 'Ard Biesheuvel (ARM address)',
'Leif Lindholm (Nuvia address)'
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@groups.io
> <bounce+27952+66927+4905953+8761045@groups.io> 代表 Laszlo Ersek
> 发送时间: 2020年11月3日 22:01
> 收件人: devel@edk2.groups.io; jbrasen@nvidia.com
> 抄送: dandan.bi@intel.com; gaoliming@byosoft.com.cn; Ard Biesheuvel
> (ARM address) <ard.biesheuvel@arm.com>; Leif Lindholm (Nuvia address)
> <leif@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@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@redhat.com>
>
> Thanks
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-05 3:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-28 17:35 [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory Jeff Brasen
2020-10-30 0:59 ` 回复: [edk2-devel] " gaoliming
2020-11-02 17:29 ` Laszlo Ersek
2020-11-03 0:56 ` 回复: " gaoliming
2020-11-03 14:01 ` Laszlo Ersek
2020-11-05 3:10 ` 回复: " gaoliming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox