* [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance
@ 2018-10-17 8:34 Jian J Wang
2018-10-17 17:57 ` Laszlo Ersek
2018-10-18 0:53 ` Dong, Eric
0 siblings, 2 replies; 5+ messages in thread
From: Jian J Wang @ 2018-10-17 8:34 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Ruiyu Ni
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;
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance
2018-10-17 8:34 [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance Jian J Wang
@ 2018-10-17 17:57 ` Laszlo Ersek
2018-10-18 0:25 ` Wang, Jian J
2018-10-18 2:06 ` Ni, Ruiyu
2018-10-18 0:53 ` Dong, Eric
1 sibling, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-10-17 17:57 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Eric Dong, Ruiyu Ni
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance
2018-10-17 17:57 ` Laszlo Ersek
@ 2018-10-18 0:25 ` Wang, Jian J
2018-10-18 2:06 ` Ni, Ruiyu
1 sibling, 0 replies; 5+ messages in thread
From: Wang, Jian J @ 2018-10-18 0:25 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric, Ni, Ruiyu
Laszlo,
Thanks for the comments. I'll update the code.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, October 18, 2018 1:57 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> 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 <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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance
2018-10-17 8:34 [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance Jian J Wang
2018-10-17 17:57 ` Laszlo Ersek
@ 2018-10-18 0:53 ` Dong, Eric
1 sibling, 0 replies; 5+ messages in thread
From: Dong, Eric @ 2018-10-18 0:53 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Ni, Ruiyu
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, October 17, 2018 4:35 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear
> descriptor data in advance
>
> 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/ArchExceptionHandle
> +++ r.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;
> --
> 2.16.2.windows.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance
2018-10-17 17:57 ` Laszlo Ersek
2018-10-18 0:25 ` Wang, Jian J
@ 2018-10-18 2:06 ` Ni, Ruiyu
1 sibling, 0 replies; 5+ messages in thread
From: Ni, Ruiyu @ 2018-10-18 2:06 UTC (permalink / raw)
To: Laszlo Ersek, Jian J Wang, edk2-devel; +Cc: Eric Dong
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-18 2:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-17 8:34 [PATCH] UefiCpuPkg/CpuExceptionHandlerLib: always clear descriptor data in advance Jian J Wang
2018-10-17 17:57 ` Laszlo Ersek
2018-10-18 0:25 ` Wang, Jian J
2018-10-18 2:06 ` Ni, Ruiyu
2018-10-18 0:53 ` Dong, Eric
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox