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.) With or without the updates: Reviewed-by: Laszlo Ersek <[email protected]> Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
