Discussion:
[edk2] [MdeModulePkg][PeiCore] Looks like there may be a PEI Core bug with Temp RAM?
Andrew Fish
2015-06-30 04:56:46 UTC
Permalink
I was playing around in an emulator and I started to see a crash with Private->Fv[0] having bad data.

I root caused the bug and it looks like if gEfiTemporaryRamSupportPpiGuid path is taken there is an assumption about the alignment of the PeiTemporaryRamBase relative to the stack.

In my usage case the SEC was allocating, “stealing”, data from the start of the traditional PeiTemporaryRamBase.

(lldb) p *SecCoreData
(EFI_SEC_PEI_HAND_OFF) $9 = {
DataSize = 0x0048
BootFirmwareVolumeBase = 0x0000000f00700000
BootFirmwareVolumeSize = 0x0000000000000000
TemporaryRamBase = 0x0000001041000000
TemporaryRamSize = 0x0000000000020000
PeiTemporaryRamBase = 0x0000001041010090
PeiTemporaryRamSize = 0x000000000000ff70
StackBase = 0x0000001041000000
StackSize = 0x0000000000010000
}

This causes PeiTemporaryRamBase to be offset by 0x90, and this seems to break the HeapOffset calculation.

https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c <https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c>
//
// TemporaryRamSupportPpi is produced by platform's SEC
//
Status = PeiServicesLocatePpi (
&gEfiTemporaryRamSupportPpiGuid,
0,
NULL,
(VOID**)&TemporaryRamSupportPpi
);
if (!EFI_ERROR (Status)) {
//
// Heap Offset
//
BaseOfNewHeap = TopOfNewStack;
if (BaseOfNewHeap >= (UINTN)SecCoreData->PeiTemporaryRamBase) {
Private->HeapOffsetPositive = TRUE;
Private->HeapOffset = (UINTN)(BaseOfNewHeap - (UINTN)SecCoreData->PeiTemporaryRamBase);
} else {
Private->HeapOffsetPositive = FALSE;
Private->HeapOffset = (UINTN)((UINTN)SecCoreData->PeiTemporaryRamBase - BaseOfNewHeap);
}
Thus the offsets look like:
(lldb) p Private->HeapOffset
(UINTN) $10 = 0x000001a04100ff70
(lldb) p Private->StackOffset
(UINTN) $11 = 0x000001a041010000

But Given the Copy via the PPI is:
TemporaryRamSupportPpi->TemporaryRamMigration (
(lldb) p TemporaryRamBase
(EFI_PHYSICAL_ADDRESS) $1 = 0x0000001041000000
(lldb) p (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize)
(EFI_PHYSICAL_ADDRESS) $0 = 0x000001b082010000
(lldb) p TemporaryRamSize
(UINTN) $2 = 0x0000000000020000

This does not work as it seems there is an broken assumption.

Old:
(lldb) p Private->Fv
(PEI_CORE_FV_HANDLE *) $3 = 0x00000010410102d8
New:
(lldb) p OldCoreData->Fv
(PEI_CORE_FV_HANDLE *) $7 = 0x000001b082020248

Thus the address is to small by 0x90 which was the offset the SEC added into the PeiTemporaryRamBase. The code seems to assume alignment requirements that are not in the specification. I see the PI Spec calls out that the stack can be a separate memory space, so I think it needs to be accounted for differently. But the HeapOffset should just calculate the offset PeiTemporaryRamBase in the TemporarayRamBase?

Thanks,

Andrew Fish

PS I verified the correct data is at the calculated correct address.

(lldb) p *(PEI_CORE_FV_HANDLE *)0x000001b0820202d8
(PEI_CORE_FV_HANDLE) $5 = {
FvHeader = 0x0000000f00700000
FvPpi = 0x0000000f00719c80
FvHandle = 0x0000000f00700000
PeimState = 0x0000001041010400 "\x01"
FvFileHandles = 0x00000010410104c8
ScanFv = true
AuthenticationStatus = 0x00000000
}

PPS. This code that is in #if 0 will show the bug. I guess it could be a bug in the EmulatorPkg, but the math seems straight forward?
https://svn.code.sf.net/p/edk2/code/trunk/edk2/EmulatorPkg/Sec/Sec.c <https://svn.code.sf.net/p/edk2/code/trunk/edk2/EmulatorPkg/Sec/Sec.c>
#if 0
// Tell the PEI Core to not use our buffer in temp RAM
SecPpiList = (EFI_PEI_PPI_DESCRIPTOR *)SecCoreData->PeiTemporaryRamBase;
SecCoreData->PeiTemporaryRamBase = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase + SecReseveredMemorySize);
SecCoreData->PeiTemporaryRamSize -= SecReseveredMemorySize;
#else
{
//
// When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
// or I don't understand temp RAM correctly?
//
EFI_PEI_PPI_DESCRIPTOR PpiArray[10];

SecPpiList = &PpiArray[0];
ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
}
#endif
Andrew Fish
2015-06-30 16:09:21 UTC
Permalink
Post by Andrew Fish
I was playing around in an emulator and I started to see a crash with Private->Fv[0] having bad data.
I root caused the bug and it looks like if gEfiTemporaryRamSupportPpiGuid path is taken there is an assumption about the alignment of the PeiTemporaryRamBase relative to the stack.
In my usage case the SEC was allocating, “stealing”, data from the start of the traditional PeiTemporaryRamBase.
(lldb) p *SecCoreData
(EFI_SEC_PEI_HAND_OFF) $9 = {
DataSize = 0x0048
BootFirmwareVolumeBase = 0x0000000f00700000
BootFirmwareVolumeSize = 0x0000000000000000
TemporaryRamBase = 0x0000001041000000
TemporaryRamSize = 0x0000000000020000
PeiTemporaryRamBase = 0x0000001041010090
PeiTemporaryRamSize = 0x000000000000ff70
StackBase = 0x0000001041000000
StackSize = 0x0000000000010000
}
This causes PeiTemporaryRamBase to be offset by 0x90, and this seems to break the HeapOffset calculation.
https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c <https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c>
//
// TemporaryRamSupportPpi is produced by platform's SEC
//
Status = PeiServicesLocatePpi (
&gEfiTemporaryRamSupportPpiGuid,
0,
NULL,
(VOID**)&TemporaryRamSupportPpi
);
if (!EFI_ERROR (Status)) {
//
// Heap Offset
//
BaseOfNewHeap = TopOfNewStack;
if (BaseOfNewHeap >= (UINTN)SecCoreData->PeiTemporaryRamBase) {
Private->HeapOffsetPositive = TRUE;
Private->HeapOffset = (UINTN)(BaseOfNewHeap - (UINTN)SecCoreData->PeiTemporaryRamBase);
} else {
Private->HeapOffsetPositive = FALSE;
Private->HeapOffset = (UINTN)((UINTN)SecCoreData->PeiTemporaryRamBase - BaseOfNewHeap);
}
Given this call:

//
// Temporary Ram Support PPI is provided by platform, it will copy
// temporary memory to permenent memory and do stack switching.
// After invoking Temporary Ram Support PPI, the following code's
// stack is in permanent memory.
//
TemporaryRamSupportPpi->TemporaryRamMigration (
PeiServices,
TemporaryRamBase,
(EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize),
TemporaryRamSize
);


It seems that in this path Private->HeapOffset is really the delta between TemporaryRamBase and (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize). This is the move that happens on the data, so it should be the heap adjustment.

Thanks,

Andrew Fish
Gao, Liming
2015-07-01 05:18:43 UTC
Permalink
Andrew:
Yes. In migration, PeiCore only considers its managed temporary memory range. It manages PeiTemporaryRam as HEAP and TemporaryStack as stack. It doesn’t know the hole data, and don’t handle it in the migration. For emulator case, the below picture shows the migration contents.



Thanks
Liming
From: Andrew Fish [mailto:***@apple.com]
Sent: Tuesday, June 30, 2015 12:57 PM
To: edk2-***@lists.sourceforge.net
Subject: [edk2] [MdeModulePkg][PeiCore] Looks like there may be a PEI Core bug with Temp RAM?

I was playing around in an emulator and I started to see a crash with Private->Fv[0] having bad data.

I root caused the bug and it looks like if gEfiTemporaryRamSupportPpiGuid path is taken there is an assumption about the alignment of the PeiTemporaryRamBase relative to the stack.

In my usage case the SEC was allocating, “stealing”, data from the start of the traditional PeiTemporaryRamBase.

(lldb) p *SecCoreData
(EFI_SEC_PEI_HAND_OFF) $9 = {
DataSize = 0x0048
BootFirmwareVolumeBase = 0x0000000f00700000
BootFirmwareVolumeSize = 0x0000000000000000
TemporaryRamBase = 0x0000001041000000
TemporaryRamSize = 0x0000000000020000
PeiTemporaryRamBase = 0x0000001041010090
PeiTemporaryRamSize = 0x000000000000ff70
StackBase = 0x0000001041000000
StackSize = 0x0000000000010000
}

This causes PeiTemporaryRamBase to be offset by 0x90, and this seems to break the HeapOffset calculation.

https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
//
// TemporaryRamSupportPpi is produced by platform's SEC
//
Status = PeiServicesLocatePpi (
&gEfiTemporaryRamSupportPpiGuid,
0,
NULL,
(VOID**)&TemporaryRamSupportPpi
);
if (!EFI_ERROR (Status)) {
//
// Heap Offset
//
BaseOfNewHeap = TopOfNewStack;
if (BaseOfNewHeap >= (UINTN)SecCoreData->PeiTemporaryRamBase) {
Private->HeapOffsetPositive = TRUE;
Private->HeapOffset = (UINTN)(BaseOfNewHeap - (UINTN)SecCoreData->PeiTemporaryRamBase);
} else {
Private->HeapOffsetPositive = FALSE;
Private->HeapOffset = (UINTN)((UINTN)SecCoreData->PeiTemporaryRamBase - BaseOfNewHeap);
}
Thus the offsets look like:
(lldb) p Private->HeapOffset
(UINTN) $10 = 0x000001a04100ff70
(lldb) p Private->StackOffset
(UINTN) $11 = 0x000001a041010000

But Given the Copy via the PPI is:
TemporaryRamSupportPpi->TemporaryRamMigration (
(lldb) p TemporaryRamBase
(EFI_PHYSICAL_ADDRESS) $1 = 0x0000001041000000
(lldb) p (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize)
(EFI_PHYSICAL_ADDRESS) $0 = 0x000001b082010000
(lldb) p TemporaryRamSize
(UINTN) $2 = 0x0000000000020000

This does not work as it seems there is an broken assumption.

Old:
(lldb) p Private->Fv
(PEI_CORE_FV_HANDLE *) $3 = 0x00000010410102d8
New:
(lldb) p OldCoreData->Fv
(PEI_CORE_FV_HANDLE *) $7 = 0x000001b082020248

Thus the address is to small by 0x90 which was the offset the SEC added into the PeiTemporaryRamBase. The code seems to assume alignment requirements that are not in the specification. I see the PI Spec calls out that the stack can be a separate memory space, so I think it needs to be accounted for differently. But the HeapOffset should just calculate the offset PeiTemporaryRamBase in the TemporarayRamBase?

Thanks,

Andrew Fish

PS I verified the correct data is at the calculated correct address.

(lldb) p *(PEI_CORE_FV_HANDLE *)0x000001b0820202d8
(PEI_CORE_FV_HANDLE) $5 = {
FvHeader = 0x0000000f00700000
FvPpi = 0x0000000f00719c80
FvHandle = 0x0000000f00700000
PeimState = 0x0000001041010400 "\x01"
FvFileHandles = 0x00000010410104c8
ScanFv = true
AuthenticationStatus = 0x00000000
}

PPS. This code that is in #if 0 will show the bug. I guess it could be a bug in the EmulatorPkg, but the math seems straight forward?
https://svn.code.sf.net/p/edk2/code/trunk/edk2/EmulatorPkg/Sec/Sec.c
#if 0
// Tell the PEI Core to not use our buffer in temp RAM
SecPpiList = (EFI_PEI_PPI_DESCRIPTOR *)SecCoreData->PeiTemporaryRamBase;
SecCoreData->PeiTemporaryRamBase = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase + SecReseveredMemorySize);
SecCoreData->PeiTemporaryRamSize -= SecReseveredMemorySize;
#else
{
//
// When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
// or I don't understand temp RAM correctly?
//
EFI_PEI_PPI_DESCRIPTOR PpiArray[10];

SecPpiList = &PpiArray[0];
ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
}
#endif

Loading...