Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance

[Laszlo Ersek <lersek@redhat.com>]

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 <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  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 <lersek@redhat.com>

Thanks
Laszlo