[edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

[Kuo, Ted]

Hi Marvin,

Good day. Thanks for your valuable comments. After checking all of your comments, I decide to drop the patches and close the bugzilla ticket since the changes should be specific to X64 in IntelFspPkg. You still can find my inline comments with [Ted] for your questions.

Thanks,
Ted

-----Original Message-----
From: Marvin Häuser <mhaeuser at posteo.de> 
Sent: Tuesday, March 22, 2022 3:46 AM
To: devel at edk2.groups.io; Kuo, Ted <ted.kuo at intel.com>
Cc: Kinney, Michael D <michael.d.kinney at intel.com>; Bi, Dandan <dandan.bi at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; De, Debkumar <debkumar.de at intel.com>; Han, Harry <harry.han at intel.com>; West, Catharine <catharine.west at intel.com>; Wang, Jian J <jian.j.wang at intel.com>
Subject: Re: [edk2-devel][PATCH 1/2] MdeModulePkg: StackOffset must be aligned to a 16-byte boundary in X64

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.kinney at intel.com>
> Cc: Dandan Bi <dandan.bi at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Debkumar De <debkumar.de at intel.com>
> Cc: Harry Han <harry.han at intel.com>
> Cc: Catharine West <catharine.west at intel.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Marvin Häuser <mhaeuser at posteo.de>
> Signed-off-by: Ted Kuo <ted.kuo at 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?
[Ted]: CPU will generate exception when running some X64 instructions which need input/output memory address to be 16-Byte-aligned.

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.
[Ted]: Since new stack must keep the original stack alignment as old stack, it means stack offset must be 16-Byte-aligned too. And the OldStack/NewStack in the fsp patch indicates the *current* old/new stack. The fsp patch just makes left shift 8-byte of whole used stack data when new stack not aligning with old stack. Hence I think no need to update TopOfNewStack.
e.g.
case1:
old stack: 0xfef5e000
new stack: 0x49c8f3b0
stack: 0x9c8f3b0 -> 16-Byte-aligned
case2: 
old stack: 0xfef5e008
new stack: 0x49c8f3b8
stack: 0x9c8f3b0 -> 16-Byte-aligned

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)
As my answer in Q2, what we adjust in the fsp patch is the new "current" stack in order to keep the same stack alignment as old stack after switching stack. Top of the new stack remains unchanged. If StackOffset is not adjusted accordingly, bios will get wrong data from Private pointer after switching to new stack.

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)
[Ted]: Actually the new stack region (from top to bottom) is not changed with the patches. It just adjusted the *current* new stack to align with the *current* old stack.

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?
[Ted]: I didn't observe the unaligned stack issue in the else case (the PPI is not found).

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)?
[Ted]: Good point. The changes should only be needed for X64. I shall make the changes specific to X64 only in IntelFspPkg. I'll drop the patch and close the bugzilla ticket.

7) This only updates FSP code, similar to 5), but to QEMU and friends continue to work?
[Ted]: The changes should be specific to X64 architecture in IntelFspPkg without any impact to QEMU and other architectures.


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 (#87818): https://edk2.groups.io/g/devel/message/87818
Mute This Topic: https://groups.io/mt/89926603/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-