On 10/18/2018 1:57 AM, Laszlo Ersek wrote:
Hi Jian,
On 10/17/18 10:34, Jian J Wang wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1237
Sometimes the memory will be contaminated by random data left in last
boot (warm reset). The code should not assume the allocated memory is
always filled with zero. This patch add code to clear data structure
used for stack switch to prevent such problem from happening.
Cc: Eric Dong <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Ruiyu Ni <[email protected]>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <[email protected]>
---
UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 3 +++
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 3 +++
2 files changed, 6 insertions(+)
diff --git
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 031d0d35fa..eebd27a25d 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -214,6 +214,7 @@ ArchSetupExcpetionStack (
//
TssBase = (UINTN)Tss;
+ TssDesc->Uint64 = 0;
TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1;
TssDesc->Bits.BaseLow = (UINT16)TssBase;
TssDesc->Bits.BaseMid = (UINT8)(TssBase >> 16);
@@ -238,6 +239,7 @@ ArchSetupExcpetionStack (
//
TssBase = (UINTN)Tss;
+ TssDesc->Uint64 = 0;
TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1;
TssDesc->Bits.BaseLow = (UINT16)TssBase;
TssDesc->Bits.BaseMid = (UINT8)(TssBase >> 16);
@@ -255,6 +257,7 @@ ArchSetupExcpetionStack (
continue;
}
+ SetMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT), 0);
Tss->EIP = (UINT32)(TemplateMap.ExceptionStart
+ Vector * TemplateMap.ExceptionStubHeaderSize);
Tss->EFLAGS = 0x2;
diff --git
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 93ecf5ae5a..6745bc77c0 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -219,6 +219,8 @@ ArchSetupExcpetionStack (
//
TssBase = (UINTN)Tss;
+ TssDesc->Uint128.Uint64 = 0;
+ TssDesc->Uint128.Uint64_1= 0;
TssDesc->Bits.LimitLow = sizeof(IA32_TASK_STATE_SEGMENT) - 1;
TssDesc->Bits.BaseLow = (UINT16)TssBase;
TssDesc->Bits.BaseMidl = (UINT8)(TssBase >> 16);
@@ -231,6 +233,7 @@ ArchSetupExcpetionStack (
//
// Fixup exception task descriptor and task-state segment
//
+ SetMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT), 0);
StackTop = StackSwitchData->X64.KnownGoodStackTop - CPU_STACK_ALIGNMENT;
StackTop = (UINTN)ALIGN_POINTER (StackTop, CPU_STACK_ALIGNMENT);
IdtTable = StackSwitchData->X64.IdtTable;
it can be checked whether this patch is complete (i.e. whether it covers
all such places) and whether it is sound (i.e. what it does is correct).
I can only offer to check the 2nd question. The patch seems correct, yes.
However, I would like to suggest two style improvements:
(1) Rather than SetMem (..., 0), I suggest ZeroMem().
(2) In general, I find
ZeroMem (Tss, sizeof *Tss);
easier to read than
ZeroMem (Tss, sizeof (IA32_TASK_STATE_SEGMENT));
If you agree, feel free to update the code before pushing. (Do await
feedback from Eric however.)
I agree both. With that, Reviewed-by: Ruiyu Ni <[email protected]>
With or without the updates:
Reviewed-by: Laszlo Ersek <[email protected]>
Thanks
Laszlo
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
