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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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 <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.)

I agree both. With that, Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

> 
> With or without the updates:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 


-- 
Thanks,
Ray