IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

[edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

Posted by Ni, Ray 6 months ago

When FSP runs in API mode, it saves the IDTR in its own stack then
switches to bootloader's stack before it returns from FspMemoryInit.
Next time when the bootloader calls TempRamExit, FSP switches to
its own stack and restores IDTR from its stack saved earlier.

However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on
FSP's stack might be corrupted that results the following TempRamExit
call fails inside FSP due to PeiServices pointer cannot be retrieved
from IDT.base - 8.

The bug is the assembly code doesn't reserve 32 bytes before calling
the C routine in 64bit. According to the x86-64 calling convention,
caller is responsible for allocating 32 bytes of "shadow space" on the
stack right before calling the function (regardless of the actual
number of parameters used).

When FSP is built in optimization-off mode, the C routine makes use
of the 32-byte "shadow space" which is not reserved by the assembly
caller. That causes the IDTR saved on the stack is corrupted by the
C routine.
The patch fixes so by reserving the 32 bytes before calling C routine.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
M: Nate DeSimone <nathaniel.l.desimone@intel.com>
M: Duggapu Chinni B <chinni.b.duggapu@intel.com>
M: Ray Han Lim Ng <ray.han.lim.ng@intel.com>
R: Star Zeng <star.zeng@intel.com>
R: Ted Kuo <ted.kuo@intel.com>
R: Ashraf Ali S <ashraf.ali.s@intel.com>
R: Susovan Mohapatra <susovan.mohapatra@intel.com>
---
 IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
index 1ea1220608..e3a7cf002f 100644
--- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
+++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
@@ -1,6 +1,6 @@
 ;------------------------------------------------------------------------------
 ;
-; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
 ; SPDX-License-Identifier: BSD-2-Clause-Patent
 ;
 ; Abstract:
@@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack):
 
     ; Load new stack
     mov     rcx, rsp
+    sub     rsp, 0x20
     call    ASM_PFX(SwapStack)
+    add     rsp, 0x20
     mov     rsp, rax
 
     ; Restore previous contexts
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384
Mute This Topic: https://groups.io/mt/102293342/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

Posted by Nate DeSimone 5 months, 3 weeks ago

Pushed as 0b4acb8.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, October 31, 2023 1:22 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>
Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit.
Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier.

However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8.

The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used).

When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine.
The patch fixes so by reserving the 32 bytes before calling C routine.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
M: Nate DeSimone <nathaniel.l.desimone@intel.com>
M: Duggapu Chinni B <chinni.b.duggapu@intel.com>
M: Ray Han Lim Ng <ray.han.lim.ng@intel.com>
R: Star Zeng <star.zeng@intel.com>
R: Ted Kuo <ted.kuo@intel.com>
R: Ashraf Ali S <ashraf.ali.s@intel.com>
R: Susovan Mohapatra <susovan.mohapatra@intel.com>
---
 IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
index 1ea1220608..e3a7cf002f 100644
--- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
+++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
@@ -1,6 +1,6 @@
 ;------------------------------------------------------------------------------ ;-; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>+; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract:@@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack):
      ; Load new stack     mov     rcx, rsp+    sub     rsp, 0x20     call    ASM_PFX(SwapStack)+    add     rsp, 0x20     mov     rsp, rax      ; Restore previous contexts-- 
2.39.1.windows.1

-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384
Mute This Topic: https://groups.io/mt/102293342/1767664
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3861758/1767664/1187970101/xyzzy [nathaniel.l.desimone@intel.com] -=-=-=-=-=-=

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110668): https://edk2.groups.io/g/devel/message/110668
Mute This Topic: https://groups.io/mt/102293342/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

Posted by Nate DeSimone 5 months, 3 weeks ago

Good catch!

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, October 31, 2023 1:22 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>
Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit

When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit.
Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier.

However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8.

The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used).

When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine.
The patch fixes so by reserving the 32 bytes before calling C routine.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
M: Nate DeSimone <nathaniel.l.desimone@intel.com>
M: Duggapu Chinni B <chinni.b.duggapu@intel.com>
M: Ray Han Lim Ng <ray.han.lim.ng@intel.com>
R: Star Zeng <star.zeng@intel.com>
R: Ted Kuo <ted.kuo@intel.com>
R: Ashraf Ali S <ashraf.ali.s@intel.com>
R: Susovan Mohapatra <susovan.mohapatra@intel.com>
---
 IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
index 1ea1220608..e3a7cf002f 100644
--- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
+++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
@@ -1,6 +1,6 @@
 ;------------------------------------------------------------------------------ ;-; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>+; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract:@@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack):
      ; Load new stack     mov     rcx, rsp+    sub     rsp, 0x20     call    ASM_PFX(SwapStack)+    add     rsp, 0x20     mov     rsp, rax      ; Restore previous contexts-- 
2.39.1.windows.1

-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384
Mute This Topic: https://groups.io/mt/102293342/1767664
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3861758/1767664/1187970101/xyzzy [nathaniel.l.desimone@intel.com] -=-=-=-=-=-=

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110667): https://edk2.groups.io/g/devel/message/110667
Mute This Topic: https://groups.io/mt/102293342/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-