Good day,
Thanks for the update!
On 21.03.22 13:43, Kuo, Ted wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865
For X64, StackOffset must be aligned to a 16-byte boundary as well as
old stack and new stack. Otherwise, it'll get wrong data from Private
pointer after switching from old stack to new stack.
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Cc: Dandan Bi <dandan...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Debkumar De <debkumar...@intel.com>
Cc: Harry Han <harry....@intel.com>
Cc: Catharine West <catharine.w...@intel.com>
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Marvin Häuser <mhaeu...@posteo.de>
Signed-off-by: Ted Kuo <ted....@intel.com>
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 3552feda8f..8a2c1ec779 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -823,6 +823,19 @@ PeiCheckAndSwitchStack (
(VOID **)&TemporaryRamSupportPpi
);
if (!EFI_ERROR (Status)) {
+ //
+ // For X64, StackOffset must be aligned to a 16-byte boundary.
Otherwise, it'll get wrong data
+ // from Private pointer after switching to new stack.
+ //
+ if ((sizeof (UINTN) == sizeof (UINT64)) && ((StackOffset & 0x0F) == 8)) {
+ if (StackOffsetPositive == TRUE) {
+ StackOffset -= 8;
+ } else {
+ StackOffset += 8;
+ }
+ Private->StackOffset = StackOffset;
+ }
+
Hmm, the overall design (not your patch) looks very broken to me. So, if
the PPI exists, it's responsible for the migration of the stack, but it
is neither passed where to migrate the stack to, nor does it return
where it did migrate it to. This means the StackOffset calculated here
may be out-of-sync with what actually happens in the PPI, e.g., if the
PPI code is changed. There also is no detailed explanation for the
memory layout with FSP separate stack vs bootloader shared stack, so I
cannot really give detailed comments quickly. *Sigh*.
Anyhow, as for the patch, I do not understand a few things:
1) Maybe most important of all, what even is broken? Which address is
not 16-Byte-aligned to cause this issue in the first place?
2) Why do you align StackOffset? Like yes, if the old top of the stack
and the offset to the new top of the stack are both 16-Byte-aligned,
then the new top of the stack is 16-Byte-aligned too. However,
StackOffset is more of a by-product and TopOfNewStack remains holding
the old value. I just don't really understand the idea of this approach.
3) This only works when StackOffset is guaranteed to be 8-Byte-aligned
(is it?). As we are dealing with the *top* of the stack (which should be
4K-aligned even for memory protection!), what would be wrong with just
aligning down and up instead?
(Same question for the second patch to the FSP code)
4) The next patch performs a similar alignment operation (as mentioned
before). However, while this patch aligns the *top* of the stack, the
FSP patch aligns the *bottom* of the stack. This may or may not be
correct based on your premises. Can you maybe document why this is
correct, or even better, try to align the top of the stack in FSP as
well? (By transitivity, if you align the top correctly, the bottom
should be aligned correctly as well, as every nested call must preserve
the alignment invariant)
5) Why does this only happen when the PPI is found? Would that not risk
an unaligned stack if the PPI is not provided, the same way it is
unaligned now?
6) The comment explicitly mentions X64, but checks only for 64-bit
pointer size. So this should affect AArch64 and RISC-V-64 as well. Are
they guaranteed to function correctly after this patch (especially with
the PPI sync guarantees mentioned earlier)?
7) This only updates FSP code, similar to 5), but to QEMU and friends
continue to work?
Thanks!
Best regards,
Marvin
//
// Heap Offset
//
@@ -852,7 +865,10 @@ PeiCheckAndSwitchStack (
// Temporary Ram Support PPI is provided by platform, it will copy
// temporary memory to permanent memory and do stack switching.
// After invoking Temporary Ram Support PPI, the following code's
- // stack is in permanent memory.
+ // stack is in permanent memory. For X64, the bit3:0 of the new stack
+ // produced by TemporaryRamMigration must be aligned with the bit3:0 of
+ // the old stack. Otherwise, it'll break the original stack alignment
+ // after switching to new stack.
//
TemporaryRamSupportPpi->TemporaryRamMigration (
PeiServices,
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#87798): https://edk2.groups.io/g/devel/message/87798
Mute This Topic: https://groups.io/mt/89926603/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
