Laszlo, Thanks for the comments. I'll update the code.
Regards, Jian > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Thursday, October 18, 2018 1:57 AM > To: Wang, Jian J <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Ni, Ruiyu <[email protected]> > Subject: Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear > descriptor data in advance > > 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
