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

["Marvin Häuser" <mhaeuser@posteo.de>]

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@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Harry Han <harry.han@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Ted Kuo <ted.kuo@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,