* [PATCH v2 1/4] MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs
2018-08-21 3:05 [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Jian J Wang
@ 2018-08-21 3:05 ` Jian J Wang
2018-08-21 3:05 ` [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler Jian J Wang
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Jian J Wang @ 2018-08-21 3:05 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Ruiyu Ni
> v2 changes:
> n/a
BIT6 of PcdHeapGuardPropertyMask is used to enable/disable non-stop mode
of Heap Guard feature. It applies to both UEFI and SMM heap guard, if
any of them is enabled.
BIT6 of PcdNullPointerDetectionPropertyMask is used to enable/disable
non-stop mode of NULL Pointer Detection feature. It applies to both
UEFI and SMM NULL Pointer Detection, if any of them is enabled.
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>
---
MdeModulePkg/MdeModulePkg.dec | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 6a6d9660ed..451115030c 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -932,7 +932,8 @@
# If enabled, accessing NULL address in UEFI or SMM code can be caught.<BR><BR>
# BIT0 - Enable NULL pointer detection for UEFI.<BR>
# BIT1 - Enable NULL pointer detection for SMM.<BR>
- # BIT2..6 - Reserved for future uses.<BR>
+ # BIT2..5 - Reserved for future uses.<BR>
+ # BIT6 - Enable non-stop mode.<BR>
# BIT7 - Disable NULL pointer detection just after EndOfDxe. <BR>
# This is a workaround for those unsolvable NULL access issues in
# OptionROM, boot loader, etc. It can also help to avoid unnecessary
@@ -1014,6 +1015,7 @@
# BIT1 - Enable UEFI pool guard.<BR>
# BIT2 - Enable SMM page guard.<BR>
# BIT3 - Enable SMM pool guard.<BR>
+ # BIT6 - Enable non-stop mode.<BR>
# BIT7 - The direction of Guard Page for Pool Guard.
# 0 - The returned pool is near the tail guard page.<BR>
# 1 - The returned pool is near the head guard page.<BR>
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
2018-08-21 3:05 [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Jian J Wang
2018-08-21 3:05 ` [PATCH v2 1/4] MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs Jian J Wang
@ 2018-08-21 3:05 ` Jian J Wang
2018-08-21 14:39 ` Laszlo Ersek
2018-08-28 1:14 ` Dong, Eric
2018-08-21 3:05 ` [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi Jian J Wang
` (2 subsequent siblings)
4 siblings, 2 replies; 15+ messages in thread
From: Jian J Wang @ 2018-08-21 3:05 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Ruiyu Ni
> v2 changes:
> n/a
Once the #PF handler has set the page to be 'present', there should
be a way to reset it to 'not-present'. 'TF' bit in EFLAGS can be used
for this purpose. 'TF' bit will be set in interrupted function context
so that it can be triggered once the cpu control returns back to the
instruction causing #PF and re-execute it.
This is an necessary step to implement non-stop mode for Heap Guard
and NULL Pointer Detection feature.
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>
---
.../Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm | 7 +++++++
.../Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm | 4 +---
.../Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 4 ++++
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
index 45d6474091..6fcf5fb23f 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
@@ -383,6 +383,13 @@ ErrorCodeAndVectorOnStack:
pop dword [ebp - 4]
mov esp, ebp
pop ebp
+
+; Enable TF bit after page fault handler runs
+ cmp dword [esp], 14 ; #PF?
+ jne .5
+ bts dword [esp + 16], 8 ; EFLAGS
+
+.5:
add esp, 8
cmp dword [esp - 16], 0 ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
jz DoReturn
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
index 62bcedea1a..7aac29c7e7 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
@@ -355,10 +355,8 @@ o16 mov [ecx + IA32_TSS._SS], ax
movzx ebx, word [ecx + IA32_TSS._CS]
mov [eax - 0x8], ebx ; create CS in old stack
mov ebx, dword [ecx + IA32_TSS.EFLAGS]
- bts ebx, 8
+ bts ebx, 8 ; Set TF
mov [eax - 0x4], ebx ; create eflags in old stack
- mov dword [ecx + IA32_TSS.EFLAGS], ebx ; update eflags in old TSS
- mov eax, dword [ecx + IA32_TSS._ESP] ; Get old stack pointer
sub eax, 0xc ; minus 12 byte
mov dword [ecx + IA32_TSS._ESP], eax ; Set new stack pointer
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
index 7b97810d10..f842af2336 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -336,6 +336,10 @@ HasErrorCode:
pop r15
mov rsp, rbp
+ cmp qword [rbp + 8], 14 ; #PF?
+ jne .1
+ bts qword [rsp + 40], 8 ; RFLAGS.TF
+.1:
pop rbp
add rsp, 16
cmp qword [rsp - 32], 0 ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
2018-08-21 3:05 ` [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler Jian J Wang
@ 2018-08-21 14:39 ` Laszlo Ersek
2018-08-28 1:14 ` Dong, Eric
1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-08-21 14:39 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Eric Dong
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> n/a
>
> Once the #PF handler has set the page to be 'present', there should
> be a way to reset it to 'not-present'. 'TF' bit in EFLAGS can be used
> for this purpose. 'TF' bit will be set in interrupted function context
> so that it can be triggered once the cpu control returns back to the
> instruction causing #PF and re-execute it.
>
> This is an necessary step to implement non-stop mode for Heap Guard
> and NULL Pointer Detection feature.
>
> 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>
> ---
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm | 7 +++++++
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm | 4 +---
> .../Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 4 ++++
> 3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
> index 45d6474091..6fcf5fb23f 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm
> @@ -383,6 +383,13 @@ ErrorCodeAndVectorOnStack:
> pop dword [ebp - 4]
> mov esp, ebp
> pop ebp
> +
> +; Enable TF bit after page fault handler runs
> + cmp dword [esp], 14 ; #PF?
> + jne .5
> + bts dword [esp + 16], 8 ; EFLAGS
> +
> +.5:
> add esp, 8
> cmp dword [esp - 16], 0 ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> jz DoReturn
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
> index 62bcedea1a..7aac29c7e7 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
> @@ -355,10 +355,8 @@ o16 mov [ecx + IA32_TSS._SS], ax
> movzx ebx, word [ecx + IA32_TSS._CS]
> mov [eax - 0x8], ebx ; create CS in old stack
> mov ebx, dword [ecx + IA32_TSS.EFLAGS]
> - bts ebx, 8
> + bts ebx, 8 ; Set TF
> mov [eax - 0x4], ebx ; create eflags in old stack
> - mov dword [ecx + IA32_TSS.EFLAGS], ebx ; update eflags in old TSS
> - mov eax, dword [ecx + IA32_TSS._ESP] ; Get old stack pointer
> sub eax, 0xc ; minus 12 byte
> mov dword [ecx + IA32_TSS._ESP], eax ; Set new stack pointer
>
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> index 7b97810d10..f842af2336 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> @@ -336,6 +336,10 @@ HasErrorCode:
> pop r15
>
> mov rsp, rbp
> + cmp qword [rbp + 8], 14 ; #PF?
> + jne .1
> + bts qword [rsp + 40], 8 ; RFLAGS.TF
> +.1:
> pop rbp
> add rsp, 16
> cmp qword [rsp - 32], 0 ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
>
I'll defer to Ray and Eric on this.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
2018-08-21 3:05 ` [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler Jian J Wang
2018-08-21 14:39 ` Laszlo Ersek
@ 2018-08-28 1:14 ` Dong, Eric
2018-08-28 3:24 ` Wang, Jian J
1 sibling, 1 reply; 15+ messages in thread
From: Dong, Eric @ 2018-08-28 1:14 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: Tuesday, August 21, 2018 11:05 AM
> 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 v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single
> step in #PF handler
>
> > v2 changes:
> > n/a
>
> Once the #PF handler has set the page to be 'present', there should be a way
> to reset it to 'not-present'. 'TF' bit in EFLAGS can be used for this purpose. 'TF'
> bit will be set in interrupted function context so that it can be triggered once
> the cpu control returns back to the instruction causing #PF and re-execute it.
>
> This is an necessary step to implement non-stop mode for Heap Guard and
> NULL Pointer Detection feature.
>
> 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>
> ---
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm | 7
> +++++++
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm | 4 +--
> -
> .../Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 4
> ++++
> 3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> index 45d6474091..6fcf5fb23f 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm
> +++ .nasm
> @@ -383,6 +383,13 @@ ErrorCodeAndVectorOnStack:
> pop dword [ebp - 4]
> mov esp, ebp
> pop ebp
> +
> +; Enable TF bit after page fault handler runs
> + cmp dword [esp], 14 ; #PF?
> + jne .5
> + bts dword [esp + 16], 8 ; EFLAGS
> +
> +.5:
> add esp, 8
> cmp dword [esp - 16], 0 ; check
> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> jz DoReturn
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> index 62bcedea1a..7aac29c7e7 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAs
> +++ m.nasm
> @@ -355,10 +355,8 @@ o16 mov [ecx + IA32_TSS._SS], ax
> movzx ebx, word [ecx + IA32_TSS._CS]
> mov [eax - 0x8], ebx ; create CS in old stack
> mov ebx, dword [ecx + IA32_TSS.EFLAGS]
> - bts ebx, 8
> + bts ebx, 8 ; Set TF
> mov [eax - 0x4], ebx ; create eflags in old stack
> - mov dword [ecx + IA32_TSS.EFLAGS], ebx ; update eflags in old TSS
> - mov eax, dword [ecx + IA32_TSS._ESP] ; Get old stack pointer
> sub eax, 0xc ; minus 12 byte
> mov dword [ecx + IA32_TSS._ESP], eax ; Set new stack pointer
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> index 7b97810d10..f842af2336 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.
> +++ nasm
> @@ -336,6 +336,10 @@ HasErrorCode:
> pop r15
>
> mov rsp, rbp
> + cmp qword [rbp + 8], 14 ; #PF?
> + jne .1
> + bts qword [rsp + 40], 8 ; RFLAGS.TF
> +.1:
> pop rbp
> add rsp, 16
> cmp qword [rsp - 32], 0 ; check
> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> --
> 2.16.2.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
2018-08-28 1:14 ` Dong, Eric
@ 2018-08-28 3:24 ` Wang, Jian J
0 siblings, 0 replies; 15+ messages in thread
From: Wang, Jian J @ 2018-08-28 3:24 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek, Ni, Ruiyu
Thanks. Since there're just some minor changes, I'll not provide v3 patches
and push the changes to master soon.
Regards,
Jian
From: Dong, Eric
Sent: Tuesday, August 28, 2018 9:15 AM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: RE: [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
Reviewed-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, August 21, 2018 11:05 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Ni,
> Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Subject: [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single
> step in #PF handler
>
> > v2 changes:
> > n/a
>
> Once the #PF handler has set the page to be 'present', there should be a way
> to reset it to 'not-present'. 'TF' bit in EFLAGS can be used for this purpose. 'TF'
> bit will be set in interrupted function context so that it can be triggered once
> the cpu control returns back to the instruction causing #PF and re-execute it.
>
> This is an necessary step to implement non-stop mode for Heap Guard and
> NULL Pointer Detection feature.
>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> ---
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.nasm | 7
> +++++++
> .../Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm | 4 +--
> -
> .../Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm | 4
> ++++
> 3 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> index 45d6474091..6fcf5fb23f 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.na
> sm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm
> +++ .nasm
> @@ -383,6 +383,13 @@ ErrorCodeAndVectorOnStack:
> pop dword [ebp - 4]
> mov esp, ebp
> pop ebp
> +
> +; Enable TF bit after page fault handler runs
> + cmp dword [esp], 14 ; #PF?
> + jne .5
> + bts dword [esp + 16], 8 ; EFLAGS
> +
> +.5:
> add esp, 8
> cmp dword [esp - 16], 0 ; check
> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> jz DoReturn
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> index 62bcedea1a..7aac29c7e7 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.n
> asm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAs
> +++ m.nasm
> @@ -355,10 +355,8 @@ o16 mov [ecx + IA32_TSS._SS], ax
> movzx ebx, word [ecx + IA32_TSS._CS]
> mov [eax - 0x8], ebx ; create CS in old stack
> mov ebx, dword [ecx + IA32_TSS.EFLAGS]
> - bts ebx, 8
> + bts ebx, 8 ; Set TF
> mov [eax - 0x4], ebx ; create eflags in old stack
> - mov dword [ecx + IA32_TSS.EFLAGS], ebx ; update eflags in old TSS
> - mov eax, dword [ecx + IA32_TSS._ESP] ; Get old stack pointer
> sub eax, 0xc ; minus 12 byte
> mov dword [ecx + IA32_TSS._ESP], eax ; Set new stack pointer
>
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> index 7b97810d10..f842af2336 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.na
> sm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.
> +++ nasm
> @@ -336,6 +336,10 @@ HasErrorCode:
> pop r15
>
> mov rsp, rbp
> + cmp qword [rbp + 8], 14 ; #PF?
> + jne .1
> + bts qword [rsp + 40], 8 ; RFLAGS.TF
> +.1:
> pop rbp
> add rsp, 16
> cmp qword [rsp - 32], 0 ; check
> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> --
> 2.16.2.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
2018-08-21 3:05 [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Jian J Wang
2018-08-21 3:05 ` [PATCH v2 1/4] MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs Jian J Wang
2018-08-21 3:05 ` [PATCH v2 2/4] UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler Jian J Wang
@ 2018-08-21 3:05 ` Jian J Wang
2018-08-21 14:58 ` Laszlo Ersek
2018-08-21 3:05 ` [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM Jian J Wang
2018-08-21 14:28 ` [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Jian J Wang @ 2018-08-21 3:05 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Ruiyu Ni
> v2 changes:
> n/a
Same as SMM profile feature, a special #PF is used to set page attribute
to 'present' and a special #DB handler to reset it back to 'not-present',
right after the instruction causing #PF got executed.
Since the new #PF handler won't enter into dead-loop, the instruction
which caused the #PF will get chance to re-execute with accessible pages.
The exception message will still be printed out on debug console so that
the developer/QA can find that there's potential heap overflow or null
pointer access occurred.
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/CpuDxe/CpuDxe.h | 39 ++++++
UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
UefiCpuPkg/CpuDxe/CpuMp.c | 34 ++++-
UefiCpuPkg/CpuDxe/CpuPageTable.c | 271 +++++++++++++++++++++++++++++++++++++++
4 files changed, 341 insertions(+), 6 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 540f5f2dbf..7d65e39e90 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -57,6 +57,12 @@
EFI_MEMORY_RO \
)
+#define HEAP_GUARD_NONSTOP_MODE \
+ ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
+
+#define NULL_DETECTION_NONSTOP_MODE \
+ ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
+
/**
Flush CPU data cache. If the instruction cache is fully coherent
with all DMA operations then function can just return EFI_SUCCESS.
@@ -273,7 +279,40 @@ RefreshGcdMemoryAttributesFromPaging (
VOID
);
+/**
+ Special handler for #DB exception, which will restore the page attributes
+ (not-present). It should work with #PF handler which will set pages to
+ 'present'.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+
+**/
+VOID
+EFIAPI
+DebugExceptionHandler (
+ IN EFI_EXCEPTION_TYPE InterruptType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ );
+
+/**
+ Special handler for #PF exception, which will set the pages which caused
+ #PF to be 'present'. The attribute of those pages should be restored in
+ the subsequent #DB handler.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+
+**/
+VOID
+EFIAPI
+PageFaultExceptionHandler (
+ IN EFI_EXCEPTION_TYPE InterruptType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ );
+
extern BOOLEAN mIsAllocatingPageTable;
+extern UINTN mNumberOfProcessors;
#endif
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
index 6a199b72f7..97a381b046 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
@@ -46,6 +46,7 @@
ReportStatusCodeLib
MpInitLib
TimerLib
+ PeCoffGetEntryPointLib
[Sources]
CpuDxe.c
@@ -79,6 +80,8 @@
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize ## CONSUMES
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index 82145e7624..f8489eb1c3 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -673,10 +673,6 @@ InitializeMpExceptionStackSwitchHandlers (
UINT8 *GdtBuffer;
UINT8 *StackTop;
- if (!PcdGetBool (PcdCpuStackGuard)) {
- return;
- }
-
ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) * ExceptionNumber;
@@ -790,6 +786,32 @@ InitializeMpExceptionStackSwitchHandlers (
}
}
+/**
+ Initializes MP exceptions handlers for special features, such as Heap Guard
+ and Stack Guard.
+**/
+VOID
+InitializeMpExceptionHandlers (
+ VOID
+ )
+{
+ //
+ // Enable non-stop mode for #PF triggered by Heap Guard or NULL Pointer
+ // Detection.
+ //
+ if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
+ RegisterCpuInterruptHandler(EXCEPT_IA32_DEBUG, DebugExceptionHandler);
+ RegisterCpuInterruptHandler(EXCEPT_IA32_PAGE_FAULT, PageFaultExceptionHandler);
+ }
+
+ //
+ // Setup stack switch for Stack Guard feature.
+ //
+ if (PcdGetBool (PcdCpuStackGuard)) {
+ InitializeMpExceptionStackSwitchHandlers();
+ }
+}
+
/**
Initialize Multi-processor support.
@@ -814,9 +836,9 @@ InitializeMpSupport (
DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
//
- // Initialize exception stack switch handlers for each logic processor.
+ // Initialize special exception handlers for each logic processor.
//
- InitializeMpExceptionStackSwitchHandlers ();
+ InitializeMpExceptionHandlers ();
//
// Update CPU healthy information from Guided HOB
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index df021798c0..b65f74bd72 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -22,6 +22,10 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/DebugLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/PeCoffGetEntryPointLib.h>
+#include <Library/SerialPortLib.h>
+#include <Library/SynchronizationLib.h>
+#include <Library/PrintLib.h>
#include <Protocol/MpService.h>
#include <Protocol/SmmBase2.h>
#include <Register/Cpuid.h>
@@ -73,6 +77,10 @@
#define PAGING_2M_ADDRESS_MASK_64 0x000FFFFFFFE00000ull
#define PAGING_1G_ADDRESS_MASK_64 0x000FFFFFC0000000ull
+#define MAX_PF_ENTRY_COUNT 10
+#define MAX_DEBUG_MESSAGE_LENGTH 0x100
+#define IA32_PF_EC_ID BIT4
+
typedef enum {
PageNone,
Page4K,
@@ -102,6 +110,13 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL;
+//
+// Record the page fault exception count for one instruction execution.
+//
+UINTN *mPFEntryCount;
+UINT64 (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];
+UINT64 *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];
+
/**
Check if current execution environment is in SMM mode or not, via
EFI_SMM_BASE2_PROTOCOL.
@@ -1135,6 +1150,253 @@ AllocatePageTableMemory (
return Buffer;
}
+/**
+ Prints a message to the serial port.
+
+ @param Format Format string for the message to print.
+ @param ... Variable argument list whose contents are accessed
+ based on the format string specified by Format.
+
+**/
+STATIC
+VOID
+EFIAPI
+InternalPrintMessage (
+ IN CONST CHAR8 *Format,
+ ...
+ )
+{
+ CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH];
+ VA_LIST Marker;
+
+ //
+ // Convert the message to an ASCII String
+ //
+ VA_START (Marker, Format);
+ AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
+ VA_END (Marker);
+
+ //
+ // Send the print string to a Serial Port
+ //
+ SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
+}
+
+/**
+ Find and display image base address and return image base and its entry point.
+
+ @param CurrentIp Current instruction pointer.
+
+**/
+STATIC
+VOID
+DumpModuleImageInfo (
+ IN UINTN CurrentIp
+ )
+{
+ EFI_STATUS Status;
+ UINTN Pe32Data;
+ VOID *PdbPointer;
+ VOID *EntryPoint;
+
+ Pe32Data = PeCoffSearchImageBase (CurrentIp);
+ if (Pe32Data == 0) {
+ InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
+ } else {
+ //
+ // Find Image Base entry point
+ //
+ Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
+ if (EFI_ERROR (Status)) {
+ EntryPoint = NULL;
+ }
+ InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentIp);
+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
+ if (PdbPointer != NULL) {
+ InternalPrintMessage ("%a", PdbPointer);
+ } else {
+ InternalPrintMessage ("(No PDB) " );
+ }
+ InternalPrintMessage (
+ " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
+ (VOID *) Pe32Data,
+ EntryPoint
+ );
+ }
+}
+
+/**
+ Special handler for #DB exception, which will restore the page attributes
+ (not-present). It should work with #PF handler which will set pages to
+ 'present'.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+
+**/
+VOID
+EFIAPI
+DebugExceptionHandler (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ )
+{
+ UINTN CpuIndex;
+ UINTN PFEntry;
+ BOOLEAN IsWpEnabled;
+
+ MpInitLibWhoAmI (&CpuIndex);
+
+ //
+ // Clear last PF entries
+ //
+ IsWpEnabled = IsReadOnlyPageWriteProtected ();
+ if (IsWpEnabled) {
+ DisableReadOnlyPageWriteProtect ();
+ }
+
+ for (PFEntry = 0; PFEntry < mPFEntryCount[CpuIndex]; PFEntry++) {
+ if (mLastPFEntryPointer[CpuIndex][PFEntry] != NULL) {
+ *mLastPFEntryPointer[CpuIndex][PFEntry] &= ~IA32_PG_P;
+ }
+ }
+
+ if (IsWpEnabled) {
+ EnableReadOnlyPageWriteProtect ();
+ }
+
+ //
+ // Reset page fault exception count for next page fault.
+ //
+ mPFEntryCount[CpuIndex] = 0;
+
+ //
+ // Flush TLB
+ //
+ CpuFlushTlb ();
+
+ //
+ // Clear TF in EFLAGS
+ //
+ if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
+ SystemContext.SystemContextIa32->Eflags &= (UINT32)~BIT8;
+ } else {
+ SystemContext.SystemContextX64->Rflags &= (UINT64)~BIT8;
+ }
+}
+
+/**
+ Special handler for #PF exception, which will set the pages which caused
+ #PF to be 'present'. The attribute of those pages should be restored in
+ the subsequent #DB handler.
+
+ @param ExceptionType Exception type.
+ @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
+
+**/
+VOID
+EFIAPI
+PageFaultExceptionHandler (
+ IN EFI_EXCEPTION_TYPE ExceptionType,
+ IN EFI_SYSTEM_CONTEXT SystemContext
+ )
+{
+ EFI_STATUS Status;
+ UINT64 PFAddress;
+ PAGE_TABLE_LIB_PAGING_CONTEXT PagingContext;
+ PAGE_ATTRIBUTE PageAttribute;
+ UINT64 Attributes;
+ UINT64 *PageEntry;
+ UINTN Index;
+ UINTN CpuIndex;
+ UINTN PageNumber;
+ UINTN CurrentIp;
+ BOOLEAN NonStopMode;
+
+ PFAddress = AsmReadCr2 () & ~EFI_PAGE_MASK;
+ if (PFAddress < BASE_4KB) {
+ NonStopMode = NULL_DETECTION_NONSTOP_MODE ? TRUE : FALSE;
+ } else {
+ NonStopMode = HEAP_GUARD_NONSTOP_MODE ? TRUE : FALSE;
+ }
+
+ if (NonStopMode) {
+ MpInitLibWhoAmI(&CpuIndex);
+ GetCurrentPagingContext (&PagingContext);
+ //
+ // Memory operation cross page boundary, like "rep mov" instruction, will
+ // cause infinite loop between this and Debug Trap handler. We have to make
+ // sure that current page and the page followed are both in PRESENT state.
+ //
+ PageNumber = 2;
+ while (PageNumber > 0) {
+ PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
+ ASSERT(PageEntry != NULL);
+
+ if (PageEntry != NULL) {
+ Attributes = GetAttributesFromPageEntry(PageEntry);
+ if ((Attributes & EFI_MEMORY_RP) != 0) {
+ Attributes &= ~EFI_MEMORY_RP;
+ Status = AssignMemoryPageAttributes (&PagingContext, PFAddress,
+ EFI_PAGE_SIZE, Attributes, NULL);
+ if (!EFI_ERROR(Status)) {
+ Index = mPFEntryCount[CpuIndex];
+ //
+ // Re-retrieve page entry because above calling might update page
+ // table due to table split.
+ //
+ PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
+ mLastPFEntryPointer[CpuIndex][Index++] = PageEntry;
+ mPFEntryCount[CpuIndex] = Index;
+ }
+ }
+ }
+
+ PFAddress += EFI_PAGE_SIZE;
+ --PageNumber;
+ }
+ }
+
+ //
+ // Initialize the serial port before dumping.
+ //
+ SerialPortInitialize ();
+ //
+ // Display ExceptionType, CPU information and Image information
+ //
+ DumpCpuContext (ExceptionType, SystemContext);
+ //
+ // Dump module image base and module entry point by RIP
+ //
+ if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
+ if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
+ //
+ // The EIP in SystemContext could not be used
+ // if it is page fault with I/D set.
+ //
+ CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp;
+ } else {
+ CurrentIp = (UINTN)SystemContext.SystemContextIa32->Eip;
+ }
+ } else {
+ if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
+ //
+ // The RIP in SystemContext could not be used
+ // if it is page fault with I/D set.
+ //
+ CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp;
+ } else {
+ CurrentIp = (UINTN)SystemContext.SystemContextX64->Rip;
+ }
+ }
+
+ DumpModuleImageInfo (CurrentIp);
+
+ if (!NonStopMode) {
+ CpuDeadLoop();
+ }
+}
+
/**
Initialize the Page Table lib.
**/
@@ -1158,6 +1420,15 @@ InitializePageTableLib (
EnableReadOnlyPageWriteProtect ();
}
+ if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
+ mPFEntryCount = (UINTN *)AllocateZeroPool (sizeof (UINTN) * mNumberOfProcessors);
+ ASSERT (mPFEntryCount != NULL);
+
+ mLastPFEntryPointer = (UINT64 *(*)[MAX_PF_ENTRY_COUNT])
+ AllocateZeroPool (sizeof (mLastPFEntryPointer[0]) * mNumberOfProcessors);
+ ASSERT (mLastPFEntryPointer != NULL);
+ }
+
DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n", CurrentPagingContext.MachineType));
DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n", CurrentPagingContext.MachineType));
DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", CurrentPagingContext.ContextData.X64.PageTableBase));
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
2018-08-21 3:05 ` [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi Jian J Wang
@ 2018-08-21 14:58 ` Laszlo Ersek
2018-08-22 7:13 ` Wang, Jian J
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-08-21 14:58 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Eric Dong
I only have some superficial comments here.
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> n/a
>
> Same as SMM profile feature, a special #PF is used to set page attribute
> to 'present' and a special #DB handler to reset it back to 'not-present',
> right after the instruction causing #PF got executed.
>
> Since the new #PF handler won't enter into dead-loop, the instruction
> which caused the #PF will get chance to re-execute with accessible pages.
>
> The exception message will still be printed out on debug console so that
> the developer/QA can find that there's potential heap overflow or null
> pointer access occurred.
>
> 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/CpuDxe/CpuDxe.h | 39 ++++++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> UefiCpuPkg/CpuDxe/CpuMp.c | 34 ++++-
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 271 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 341 insertions(+), 6 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 540f5f2dbf..7d65e39e90 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -57,6 +57,12 @@
> EFI_MEMORY_RO \
> )
>
> +#define HEAP_GUARD_NONSTOP_MODE \
> + ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
> +
> +#define NULL_DETECTION_NONSTOP_MODE \
> + ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
> +
> /**
> Flush CPU data cache. If the instruction cache is fully coherent
> with all DMA operations then function can just return EFI_SUCCESS.
> @@ -273,7 +279,40 @@ RefreshGcdMemoryAttributesFromPaging (
> VOID
> );
>
> +/**
> + Special handler for #DB exception, which will restore the page attributes
> + (not-present). It should work with #PF handler which will set pages to
> + 'present'.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugExceptionHandler (
> + IN EFI_EXCEPTION_TYPE InterruptType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + );
> +
> +/**
> + Special handler for #PF exception, which will set the pages which caused
> + #PF to be 'present'. The attribute of those pages should be restored in
> + the subsequent #DB handler.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +PageFaultExceptionHandler (
> + IN EFI_EXCEPTION_TYPE InterruptType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + );
> +
> extern BOOLEAN mIsAllocatingPageTable;
> +extern UINTN mNumberOfProcessors;
>
> #endif
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 6a199b72f7..97a381b046 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -46,6 +46,7 @@
> ReportStatusCodeLib
> MpInitLib
> TimerLib
> + PeCoffGetEntryPointLib
>
> [Sources]
> CpuDxe.c
> @@ -79,6 +80,8 @@
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize ## CONSUMES
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 82145e7624..f8489eb1c3 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -673,10 +673,6 @@ InitializeMpExceptionStackSwitchHandlers (
> UINT8 *GdtBuffer;
> UINT8 *StackTop;
>
> - if (!PcdGetBool (PcdCpuStackGuard)) {
> - return;
> - }
> -
> ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) * ExceptionNumber;
>
> @@ -790,6 +786,32 @@ InitializeMpExceptionStackSwitchHandlers (
> }
> }
>
> +/**
> + Initializes MP exceptions handlers for special features, such as Heap Guard
> + and Stack Guard.
> +**/
> +VOID
> +InitializeMpExceptionHandlers (
> + VOID
> + )
> +{
> + //
> + // Enable non-stop mode for #PF triggered by Heap Guard or NULL Pointer
> + // Detection.
> + //
> + if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
> + RegisterCpuInterruptHandler(EXCEPT_IA32_DEBUG, DebugExceptionHandler);
> + RegisterCpuInterruptHandler(EXCEPT_IA32_PAGE_FAULT, PageFaultExceptionHandler);
> + }
> +
> + //
> + // Setup stack switch for Stack Guard feature.
> + //
> + if (PcdGetBool (PcdCpuStackGuard)) {
> + InitializeMpExceptionStackSwitchHandlers();
> + }
> +}
> +
> /**
> Initialize Multi-processor support.
>
> @@ -814,9 +836,9 @@ InitializeMpSupport (
> DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
>
> //
> - // Initialize exception stack switch handlers for each logic processor.
> + // Initialize special exception handlers for each logic processor.
> //
> - InitializeMpExceptionStackSwitchHandlers ();
> + InitializeMpExceptionHandlers ();
>
> //
> // Update CPU healthy information from Guided HOB
The reworking of the InitializeMpExceptionStackSwitchHandlers() /
PcdGetBool (PcdCpuStackGuard) call sites look OK to me.
(1) However, some whitespace is missing before opening parentheses. See
eg. after "RegisterCpuInterruptHandler".
Please check the rest of the code for that as well.
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index df021798c0..b65f74bd72 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -22,6 +22,10 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/DebugLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/PeCoffGetEntryPointLib.h>
> +#include <Library/SerialPortLib.h>
> +#include <Library/SynchronizationLib.h>
> +#include <Library/PrintLib.h>
> #include <Protocol/MpService.h>
> #include <Protocol/SmmBase2.h>
> #include <Register/Cpuid.h>
> @@ -73,6 +77,10 @@
> #define PAGING_2M_ADDRESS_MASK_64 0x000FFFFFFFE00000ull
> #define PAGING_1G_ADDRESS_MASK_64 0x000FFFFFC0000000ull
>
> +#define MAX_PF_ENTRY_COUNT 10
> +#define MAX_DEBUG_MESSAGE_LENGTH 0x100
> +#define IA32_PF_EC_ID BIT4
> +
> typedef enum {
> PageNone,
> Page4K,
> @@ -102,6 +110,13 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
> EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL;
>
> +//
> +// Record the page fault exception count for one instruction execution.
> +//
> +UINTN *mPFEntryCount;
> +UINT64 (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];
(2) "mLastPFEntryValue" seems unused.
> +UINT64 *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];
(3) I think mPFEntryCount and mLastPFEntryPointer could be made STATIC.
(Not sure if that's consistent with the rest of the variable definitions
though, in this file.)
> +
> /**
> Check if current execution environment is in SMM mode or not, via
> EFI_SMM_BASE2_PROTOCOL.
> @@ -1135,6 +1150,253 @@ AllocatePageTableMemory (
> return Buffer;
> }
>
> +/**
> + Prints a message to the serial port.
> +
> + @param Format Format string for the message to print.
> + @param ... Variable argument list whose contents are accessed
> + based on the format string specified by Format.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +InternalPrintMessage (
> + IN CONST CHAR8 *Format,
> + ...
> + )
> +{
> + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> + VA_LIST Marker;
> +
> + //
> + // Convert the message to an ASCII String
> + //
> + VA_START (Marker, Format);
> + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
> + VA_END (Marker);
> +
> + //
> + // Send the print string to a Serial Port
> + //
> + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> +}
> +
> +/**
> + Find and display image base address and return image base and its entry point.
> +
> + @param CurrentIp Current instruction pointer.
> +
> +**/
> +STATIC
> +VOID
> +DumpModuleImageInfo (
> + IN UINTN CurrentIp
> + )
> +{
> + EFI_STATUS Status;
> + UINTN Pe32Data;
> + VOID *PdbPointer;
> + VOID *EntryPoint;
> +
> + Pe32Data = PeCoffSearchImageBase (CurrentIp);
> + if (Pe32Data == 0) {
> + InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
> + } else {
> + //
> + // Find Image Base entry point
> + //
> + Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
> + if (EFI_ERROR (Status)) {
> + EntryPoint = NULL;
> + }
> + InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentIp);
> + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
> + if (PdbPointer != NULL) {
> + InternalPrintMessage ("%a", PdbPointer);
> + } else {
> + InternalPrintMessage ("(No PDB) " );
> + }
> + InternalPrintMessage (
> + " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
> + (VOID *) Pe32Data,
> + EntryPoint
> + );
> + }
> +}
(4) Why is this function copied here? From a quick check, it looks
identical to DumpModuleImageInfo() in
"UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c".
And, DumpModuleImageInfo() is an extern function in
"UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h", and we
are (likely) already linking against that library instance. As a result
we'll have the same function twice in CpuDxe, once as STATIC, and
another time as extern.
If this is a useful utility function, then it should be elevated to a
public library API, and used from there. (I don't know where to add it,
I just find this code duplication regrettable.)
Thanks
Laszlo
> +
> +/**
> + Special handler for #DB exception, which will restore the page attributes
> + (not-present). It should work with #PF handler which will set pages to
> + 'present'.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugExceptionHandler (
> + IN EFI_EXCEPTION_TYPE ExceptionType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + UINTN CpuIndex;
> + UINTN PFEntry;
> + BOOLEAN IsWpEnabled;
> +
> + MpInitLibWhoAmI (&CpuIndex);
> +
> + //
> + // Clear last PF entries
> + //
> + IsWpEnabled = IsReadOnlyPageWriteProtected ();
> + if (IsWpEnabled) {
> + DisableReadOnlyPageWriteProtect ();
> + }
> +
> + for (PFEntry = 0; PFEntry < mPFEntryCount[CpuIndex]; PFEntry++) {
> + if (mLastPFEntryPointer[CpuIndex][PFEntry] != NULL) {
> + *mLastPFEntryPointer[CpuIndex][PFEntry] &= ~IA32_PG_P;
> + }
> + }
> +
> + if (IsWpEnabled) {
> + EnableReadOnlyPageWriteProtect ();
> + }
> +
> + //
> + // Reset page fault exception count for next page fault.
> + //
> + mPFEntryCount[CpuIndex] = 0;
> +
> + //
> + // Flush TLB
> + //
> + CpuFlushTlb ();
> +
> + //
> + // Clear TF in EFLAGS
> + //
> + if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
> + SystemContext.SystemContextIa32->Eflags &= (UINT32)~BIT8;
> + } else {
> + SystemContext.SystemContextX64->Rflags &= (UINT64)~BIT8;
> + }
> +}
> +
> +/**
> + Special handler for #PF exception, which will set the pages which caused
> + #PF to be 'present'. The attribute of those pages should be restored in
> + the subsequent #DB handler.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +PageFaultExceptionHandler (
> + IN EFI_EXCEPTION_TYPE ExceptionType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + EFI_STATUS Status;
> + UINT64 PFAddress;
> + PAGE_TABLE_LIB_PAGING_CONTEXT PagingContext;
> + PAGE_ATTRIBUTE PageAttribute;
> + UINT64 Attributes;
> + UINT64 *PageEntry;
> + UINTN Index;
> + UINTN CpuIndex;
> + UINTN PageNumber;
> + UINTN CurrentIp;
> + BOOLEAN NonStopMode;
> +
> + PFAddress = AsmReadCr2 () & ~EFI_PAGE_MASK;
> + if (PFAddress < BASE_4KB) {
> + NonStopMode = NULL_DETECTION_NONSTOP_MODE ? TRUE : FALSE;
> + } else {
> + NonStopMode = HEAP_GUARD_NONSTOP_MODE ? TRUE : FALSE;
> + }
> +
> + if (NonStopMode) {
> + MpInitLibWhoAmI(&CpuIndex);
> + GetCurrentPagingContext (&PagingContext);
> + //
> + // Memory operation cross page boundary, like "rep mov" instruction, will
> + // cause infinite loop between this and Debug Trap handler. We have to make
> + // sure that current page and the page followed are both in PRESENT state.
> + //
> + PageNumber = 2;
> + while (PageNumber > 0) {
> + PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
> + ASSERT(PageEntry != NULL);
> +
> + if (PageEntry != NULL) {
> + Attributes = GetAttributesFromPageEntry(PageEntry);
> + if ((Attributes & EFI_MEMORY_RP) != 0) {
> + Attributes &= ~EFI_MEMORY_RP;
> + Status = AssignMemoryPageAttributes (&PagingContext, PFAddress,
> + EFI_PAGE_SIZE, Attributes, NULL);
> + if (!EFI_ERROR(Status)) {
> + Index = mPFEntryCount[CpuIndex];
> + //
> + // Re-retrieve page entry because above calling might update page
> + // table due to table split.
> + //
> + PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
> + mLastPFEntryPointer[CpuIndex][Index++] = PageEntry;
> + mPFEntryCount[CpuIndex] = Index;
> + }
> + }
> + }
> +
> + PFAddress += EFI_PAGE_SIZE;
> + --PageNumber;
> + }
> + }
> +
> + //
> + // Initialize the serial port before dumping.
> + //
> + SerialPortInitialize ();
> + //
> + // Display ExceptionType, CPU information and Image information
> + //
> + DumpCpuContext (ExceptionType, SystemContext);
> + //
> + // Dump module image base and module entry point by RIP
> + //
> + if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
> + if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
> + //
> + // The EIP in SystemContext could not be used
> + // if it is page fault with I/D set.
> + //
> + CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp;
> + } else {
> + CurrentIp = (UINTN)SystemContext.SystemContextIa32->Eip;
> + }
> + } else {
> + if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
> + //
> + // The RIP in SystemContext could not be used
> + // if it is page fault with I/D set.
> + //
> + CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp;
> + } else {
> + CurrentIp = (UINTN)SystemContext.SystemContextX64->Rip;
> + }
> + }
> +
> + DumpModuleImageInfo (CurrentIp);
> +
> + if (!NonStopMode) {
> + CpuDeadLoop();
> + }
> +}
> +
> /**
> Initialize the Page Table lib.
> **/
> @@ -1158,6 +1420,15 @@ InitializePageTableLib (
> EnableReadOnlyPageWriteProtect ();
> }
>
> + if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
> + mPFEntryCount = (UINTN *)AllocateZeroPool (sizeof (UINTN) * mNumberOfProcessors);
> + ASSERT (mPFEntryCount != NULL);
> +
> + mLastPFEntryPointer = (UINT64 *(*)[MAX_PF_ENTRY_COUNT])
> + AllocateZeroPool (sizeof (mLastPFEntryPointer[0]) * mNumberOfProcessors);
> + ASSERT (mLastPFEntryPointer != NULL);
> + }
> +
> DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n", CurrentPagingContext.MachineType));
> DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n", CurrentPagingContext.MachineType));
> DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", CurrentPagingContext.ContextData.X64.PageTableBase));
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
2018-08-21 14:58 ` Laszlo Ersek
@ 2018-08-22 7:13 ` Wang, Jian J
2018-08-22 7:45 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Wang, Jian J @ 2018-08-22 7:13 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Dong, Eric
Hi Laszlo,
Regards,
Jian
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, August 21, 2018 10:59 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
I only have some superficial comments here.
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> n/a
>
> Same as SMM profile feature, a special #PF is used to set page attribute
> to 'present' and a special #DB handler to reset it back to 'not-present',
> right after the instruction causing #PF got executed.
>
> Since the new #PF handler won't enter into dead-loop, the instruction
> which caused the #PF will get chance to re-execute with accessible pages.
>
> The exception message will still be printed out on debug console so that
> the developer/QA can find that there's potential heap overflow or null
> pointer access occurred.
>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.h | 39 ++++++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> UefiCpuPkg/CpuDxe/CpuMp.c | 34 ++++-
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 271 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 341 insertions(+), 6 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 540f5f2dbf..7d65e39e90 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -57,6 +57,12 @@
> EFI_MEMORY_RO \
> )
>
> +#define HEAP_GUARD_NONSTOP_MODE \
> + ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
> +
> +#define NULL_DETECTION_NONSTOP_MODE \
> + ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
> +
> /**
> Flush CPU data cache. If the instruction cache is fully coherent
> with all DMA operations then function can just return EFI_SUCCESS.
> @@ -273,7 +279,40 @@ RefreshGcdMemoryAttributesFromPaging (
> VOID
> );
>
> +/**
> + Special handler for #DB exception, which will restore the page attributes
> + (not-present). It should work with #PF handler which will set pages to
> + 'present'.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugExceptionHandler (
> + IN EFI_EXCEPTION_TYPE InterruptType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + );
> +
> +/**
> + Special handler for #PF exception, which will set the pages which caused
> + #PF to be 'present'. The attribute of those pages should be restored in
> + the subsequent #DB handler.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +PageFaultExceptionHandler (
> + IN EFI_EXCEPTION_TYPE InterruptType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + );
> +
> extern BOOLEAN mIsAllocatingPageTable;
> +extern UINTN mNumberOfProcessors;
>
> #endif
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 6a199b72f7..97a381b046 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -46,6 +46,7 @@
> ReportStatusCodeLib
> MpInitLib
> TimerLib
> + PeCoffGetEntryPointLib
>
> [Sources]
> CpuDxe.c
> @@ -79,6 +80,8 @@
> [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList ## CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize ## CONSUMES
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 82145e7624..f8489eb1c3 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -673,10 +673,6 @@ InitializeMpExceptionStackSwitchHandlers (
> UINT8 *GdtBuffer;
> UINT8 *StackTop;
>
> - if (!PcdGetBool (PcdCpuStackGuard)) {
> - return;
> - }
> -
> ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) * ExceptionNumber;
>
> @@ -790,6 +786,32 @@ InitializeMpExceptionStackSwitchHandlers (
> }
> }
>
> +/**
> + Initializes MP exceptions handlers for special features, such as Heap Guard
> + and Stack Guard.
> +**/
> +VOID
> +InitializeMpExceptionHandlers (
> + VOID
> + )
> +{
> + //
> + // Enable non-stop mode for #PF triggered by Heap Guard or NULL Pointer
> + // Detection.
> + //
> + if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
> + RegisterCpuInterruptHandler(EXCEPT_IA32_DEBUG, DebugExceptionHandler);
> + RegisterCpuInterruptHandler(EXCEPT_IA32_PAGE_FAULT, PageFaultExceptionHandler);
> + }
> +
> + //
> + // Setup stack switch for Stack Guard feature.
> + //
> + if (PcdGetBool (PcdCpuStackGuard)) {
> + InitializeMpExceptionStackSwitchHandlers();
> + }
> +}
> +
> /**
> Initialize Multi-processor support.
>
> @@ -814,9 +836,9 @@ InitializeMpSupport (
> DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
>
> //
> - // Initialize exception stack switch handlers for each logic processor.
> + // Initialize special exception handlers for each logic processor.
> //
> - InitializeMpExceptionStackSwitchHandlers ();
> + InitializeMpExceptionHandlers ();
>
> //
> // Update CPU healthy information from Guided HOB
The reworking of the InitializeMpExceptionStackSwitchHandlers() /
PcdGetBool (PcdCpuStackGuard) call sites look OK to me.
(1) However, some whitespace is missing before opening parentheses. See
eg. after "RegisterCpuInterruptHandler".
Please check the rest of the code for that as well.
[Jian] Sure. I’ll check it.
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index df021798c0..b65f74bd72 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -22,6 +22,10 @@
> #include <Library/MemoryAllocationLib.h>
> #include <Library/DebugLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/PeCoffGetEntryPointLib.h>
> +#include <Library/SerialPortLib.h>
> +#include <Library/SynchronizationLib.h>
> +#include <Library/PrintLib.h>
> #include <Protocol/MpService.h>
> #include <Protocol/SmmBase2.h>
> #include <Register/Cpuid.h>
> @@ -73,6 +77,10 @@
> #define PAGING_2M_ADDRESS_MASK_64 0x000FFFFFFFE00000ull
> #define PAGING_1G_ADDRESS_MASK_64 0x000FFFFFC0000000ull
>
> +#define MAX_PF_ENTRY_COUNT 10
> +#define MAX_DEBUG_MESSAGE_LENGTH 0x100
> +#define IA32_PF_EC_ID BIT4
> +
> typedef enum {
> PageNone,
> Page4K,
> @@ -102,6 +110,13 @@ PAGE_TABLE_POOL *mPageTablePool = NULL;
> PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
> EFI_SMM_BASE2_PROTOCOL *mSmmBase2 = NULL;
>
> +//
> +// Record the page fault exception count for one instruction execution.
> +//
> +UINTN *mPFEntryCount;
> +UINT64 (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];
(2) "mLastPFEntryValue" seems unused.
[Jian] You’re right. It should be removed.
> +UINT64 *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];
(3) I think mPFEntryCount and mLastPFEntryPointer could be made STATIC.
(Not sure if that's consistent with the rest of the variable definitions
though, in this file.)
[Jian] I double checked. No STATIC used. So let’s keep it as is.
> +
> /**
> Check if current execution environment is in SMM mode or not, via
> EFI_SMM_BASE2_PROTOCOL.
> @@ -1135,6 +1150,253 @@ AllocatePageTableMemory (
> return Buffer;
> }
>
> +/**
> + Prints a message to the serial port.
> +
> + @param Format Format string for the message to print.
> + @param ... Variable argument list whose contents are accessed
> + based on the format string specified by Format.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +InternalPrintMessage (
> + IN CONST CHAR8 *Format,
> + ...
> + )
> +{
> + CHAR8 Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> + VA_LIST Marker;
> +
> + //
> + // Convert the message to an ASCII String
> + //
> + VA_START (Marker, Format);
> + AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
> + VA_END (Marker);
> +
> + //
> + // Send the print string to a Serial Port
> + //
> + SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> +}
> +
> +/**
> + Find and display image base address and return image base and its entry point.
> +
> + @param CurrentIp Current instruction pointer.
> +
> +**/
> +STATIC
> +VOID
> +DumpModuleImageInfo (
> + IN UINTN CurrentIp
> + )
> +{
> + EFI_STATUS Status;
> + UINTN Pe32Data;
> + VOID *PdbPointer;
> + VOID *EntryPoint;
> +
> + Pe32Data = PeCoffSearchImageBase (CurrentIp);
> + if (Pe32Data == 0) {
> + InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
> + } else {
> + //
> + // Find Image Base entry point
> + //
> + Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
> + if (EFI_ERROR (Status)) {
> + EntryPoint = NULL;
> + }
> + InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentIp);
> + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
> + if (PdbPointer != NULL) {
> + InternalPrintMessage ("%a", PdbPointer);
> + } else {
> + InternalPrintMessage ("(No PDB) " );
> + }
> + InternalPrintMessage (
> + " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
> + (VOID *) Pe32Data,
> + EntryPoint
> + );
> + }
> +}
(4) Why is this function copied here? From a quick check, it looks
identical to DumpModuleImageInfo() in
"UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c".
And, DumpModuleImageInfo() is an extern function in
"UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h", and we
are (likely) already linking against that library instance. As a result
we'll have the same function twice in CpuDxe, once as STATIC, and
another time as extern.
If this is a useful utility function, then it should be elevated to a
public library API, and used from there. (I don't know where to add it,
I just find this code duplication regrettable.)
[Jian] I reviewed the whole working model again. I think we don’t need to dump
the image information here, because, if there’s an issue detected, the developers
will disable the non-stop mode anyway in order to find the root cause. Then the
image information can be dumped (normal mode). So we can just dump cpu
information to tell the user that there’s an exception here. And dumping cpu
information is already a public interface so we don’t need to duplicate any code
here.
Thanks
Laszlo
> +
> +/**
> + Special handler for #DB exception, which will restore the page attributes
> + (not-present). It should work with #PF handler which will set pages to
> + 'present'.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugExceptionHandler (
> + IN EFI_EXCEPTION_TYPE ExceptionType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + UINTN CpuIndex;
> + UINTN PFEntry;
> + BOOLEAN IsWpEnabled;
> +
> + MpInitLibWhoAmI (&CpuIndex);
> +
> + //
> + // Clear last PF entries
> + //
> + IsWpEnabled = IsReadOnlyPageWriteProtected ();
> + if (IsWpEnabled) {
> + DisableReadOnlyPageWriteProtect ();
> + }
> +
> + for (PFEntry = 0; PFEntry < mPFEntryCount[CpuIndex]; PFEntry++) {
> + if (mLastPFEntryPointer[CpuIndex][PFEntry] != NULL) {
> + *mLastPFEntryPointer[CpuIndex][PFEntry] &= ~IA32_PG_P;
> + }
> + }
> +
> + if (IsWpEnabled) {
> + EnableReadOnlyPageWriteProtect ();
> + }
> +
> + //
> + // Reset page fault exception count for next page fault.
> + //
> + mPFEntryCount[CpuIndex] = 0;
> +
> + //
> + // Flush TLB
> + //
> + CpuFlushTlb ();
> +
> + //
> + // Clear TF in EFLAGS
> + //
> + if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
> + SystemContext.SystemContextIa32->Eflags &= (UINT32)~BIT8;
> + } else {
> + SystemContext.SystemContextX64->Rflags &= (UINT64)~BIT8;
> + }
> +}
> +
> +/**
> + Special handler for #PF exception, which will set the pages which caused
> + #PF to be 'present'. The attribute of those pages should be restored in
> + the subsequent #DB handler.
> +
> + @param ExceptionType Exception type.
> + @param SystemContext Pointer to EFI_SYSTEM_CONTEXT.
> +
> +**/
> +VOID
> +EFIAPI
> +PageFaultExceptionHandler (
> + IN EFI_EXCEPTION_TYPE ExceptionType,
> + IN EFI_SYSTEM_CONTEXT SystemContext
> + )
> +{
> + EFI_STATUS Status;
> + UINT64 PFAddress;
> + PAGE_TABLE_LIB_PAGING_CONTEXT PagingContext;
> + PAGE_ATTRIBUTE PageAttribute;
> + UINT64 Attributes;
> + UINT64 *PageEntry;
> + UINTN Index;
> + UINTN CpuIndex;
> + UINTN PageNumber;
> + UINTN CurrentIp;
> + BOOLEAN NonStopMode;
> +
> + PFAddress = AsmReadCr2 () & ~EFI_PAGE_MASK;
> + if (PFAddress < BASE_4KB) {
> + NonStopMode = NULL_DETECTION_NONSTOP_MODE ? TRUE : FALSE;
> + } else {
> + NonStopMode = HEAP_GUARD_NONSTOP_MODE ? TRUE : FALSE;
> + }
> +
> + if (NonStopMode) {
> + MpInitLibWhoAmI(&CpuIndex);
> + GetCurrentPagingContext (&PagingContext);
> + //
> + // Memory operation cross page boundary, like "rep mov" instruction, will
> + // cause infinite loop between this and Debug Trap handler. We have to make
> + // sure that current page and the page followed are both in PRESENT state.
> + //
> + PageNumber = 2;
> + while (PageNumber > 0) {
> + PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
> + ASSERT(PageEntry != NULL);
> +
> + if (PageEntry != NULL) {
> + Attributes = GetAttributesFromPageEntry(PageEntry);
> + if ((Attributes & EFI_MEMORY_RP) != 0) {
> + Attributes &= ~EFI_MEMORY_RP;
> + Status = AssignMemoryPageAttributes (&PagingContext, PFAddress,
> + EFI_PAGE_SIZE, Attributes, NULL);
> + if (!EFI_ERROR(Status)) {
> + Index = mPFEntryCount[CpuIndex];
> + //
> + // Re-retrieve page entry because above calling might update page
> + // table due to table split.
> + //
> + PageEntry = GetPageTableEntry(&PagingContext, PFAddress, &PageAttribute);
> + mLastPFEntryPointer[CpuIndex][Index++] = PageEntry;
> + mPFEntryCount[CpuIndex] = Index;
> + }
> + }
> + }
> +
> + PFAddress += EFI_PAGE_SIZE;
> + --PageNumber;
> + }
> + }
> +
> + //
> + // Initialize the serial port before dumping.
> + //
> + SerialPortInitialize ();
> + //
> + // Display ExceptionType, CPU information and Image information
> + //
> + DumpCpuContext (ExceptionType, SystemContext);
> + //
> + // Dump module image base and module entry point by RIP
> + //
> + if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
> + if ((SystemContext.SystemContextIa32->ExceptionData & IA32_PF_EC_ID) != 0) {
> + //
> + // The EIP in SystemContext could not be used
> + // if it is page fault with I/D set.
> + //
> + CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextIa32->Esp;
> + } else {
> + CurrentIp = (UINTN)SystemContext.SystemContextIa32->Eip;
> + }
> + } else {
> + if ((SystemContext.SystemContextX64->ExceptionData & IA32_PF_EC_ID) != 0) {
> + //
> + // The RIP in SystemContext could not be used
> + // if it is page fault with I/D set.
> + //
> + CurrentIp = *(UINTN *)(UINTN)SystemContext.SystemContextX64->Rsp;
> + } else {
> + CurrentIp = (UINTN)SystemContext.SystemContextX64->Rip;
> + }
> + }
> +
> + DumpModuleImageInfo (CurrentIp);
> +
> + if (!NonStopMode) {
> + CpuDeadLoop();
> + }
> +}
> +
> /**
> Initialize the Page Table lib.
> **/
> @@ -1158,6 +1420,15 @@ InitializePageTableLib (
> EnableReadOnlyPageWriteProtect ();
> }
>
> + if (HEAP_GUARD_NONSTOP_MODE || NULL_DETECTION_NONSTOP_MODE) {
> + mPFEntryCount = (UINTN *)AllocateZeroPool (sizeof (UINTN) * mNumberOfProcessors);
> + ASSERT (mPFEntryCount != NULL);
> +
> + mLastPFEntryPointer = (UINT64 *(*)[MAX_PF_ENTRY_COUNT])
> + AllocateZeroPool (sizeof (mLastPFEntryPointer[0]) * mNumberOfProcessors);
> + ASSERT (mLastPFEntryPointer != NULL);
> + }
> +
> DEBUG ((DEBUG_INFO, "CurrentPagingContext:\n", CurrentPagingContext.MachineType));
> DEBUG ((DEBUG_INFO, " MachineType - 0x%x\n", CurrentPagingContext.MachineType));
> DEBUG ((DEBUG_INFO, " PageTableBase - 0x%x\n", CurrentPagingContext.ContextData.X64.PageTableBase));
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
2018-08-22 7:13 ` Wang, Jian J
@ 2018-08-22 7:45 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-08-22 7:45 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Dong, Eric
On 08/22/18 09:13, Wang, Jian J wrote:
> Hi Laszlo,
>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, August 21, 2018 10:59 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
>> +/**
>> + Find and display image base address and return image base and its entry point.
>> +
>> + @param CurrentIp Current instruction pointer.
>> +
>> +**/
>> +STATIC
>> +VOID
>> +DumpModuleImageInfo (
>> + IN UINTN CurrentIp
>> + )
>> +{
>> + EFI_STATUS Status;
>> + UINTN Pe32Data;
>> + VOID *PdbPointer;
>> + VOID *EntryPoint;
>> +
>> + Pe32Data = PeCoffSearchImageBase (CurrentIp);
>> + if (Pe32Data == 0) {
>> + InternalPrintMessage ("!!!! Can't find image information. !!!!\n");
>> + } else {
>> + //
>> + // Find Image Base entry point
>> + //
>> + Status = PeCoffLoaderGetEntryPoint ((VOID *) Pe32Data, &EntryPoint);
>> + if (EFI_ERROR (Status)) {
>> + EntryPoint = NULL;
>> + }
>> + InternalPrintMessage ("!!!! Find image based on IP(0x%x) ", CurrentIp);
>> + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *) Pe32Data);
>> + if (PdbPointer != NULL) {
>> + InternalPrintMessage ("%a", PdbPointer);
>> + } else {
>> + InternalPrintMessage ("(No PDB) " );
>> + }
>> + InternalPrintMessage (
>> + " (ImageBase=%016lp, EntryPoint=%016p) !!!!\n",
>> + (VOID *) Pe32Data,
>> + EntryPoint
>> + );
>> + }
>> +}
>
> (4) Why is this function copied here? From a quick check, it looks
> identical to DumpModuleImageInfo() in
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c".
>
> And, DumpModuleImageInfo() is an extern function in
> "UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h", and we
> are (likely) already linking against that library instance. As a result
> we'll have the same function twice in CpuDxe, once as STATIC, and
> another time as extern.
>
> If this is a useful utility function, then it should be elevated to a
> public library API, and used from there. (I don't know where to add it,
> I just find this code duplication regrettable.)
>
> [Jian] I reviewed the whole working model again. I think we don’t need to dump
> the image information here, because, if there’s an issue detected, the developers
> will disable the non-stop mode anyway in order to find the root cause. Then the
> image information can be dumped (normal mode). So we can just dump cpu
> information to tell the user that there’s an exception here. And dumping cpu
> information is already a public interface so we don’t need to duplicate any code
> here.
Oh cool. If we can eliminate the function altogether, that's best.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
2018-08-21 3:05 [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Jian J Wang
` (2 preceding siblings ...)
2018-08-21 3:05 ` [PATCH v2 3/4] UefiCpuPkg/CpuDxe: implement non-stop mode for uefi Jian J Wang
@ 2018-08-21 3:05 ` Jian J Wang
2018-08-21 15:44 ` Laszlo Ersek
2018-08-21 14:28 ` [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Jian J Wang @ 2018-08-21 3:05 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Ruiyu Ni
> v2 changes:
> fix GCC build error caused by an unused variable in GuardPagePFHandler()
Since SMM profile feature has already implemented non-stop mode if #PF
occurred, this patch just makes use of the existing implementation to
accommodate heap guard and NULL pointer detection feature.
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/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 43 ++++++++++++------
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 3 +-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 58 +++++++++++++++++++++++-
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15 ++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 6 +++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 43 ++++++++++++------
6 files changed, 137 insertions(+), 31 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index 9300a232e4..a32b736089 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -38,7 +38,9 @@ SmmInitPageTable (
mPhysicalAddressBits = 32;
- if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
+ if (FeaturePcdGet (PcdCpuSmmProfileEnable) ||
+ HEAP_GUARD_NONSTOP_MODE ||
+ NULL_DETECTION_NONSTOP_MODE) {
//
// Set own Page Fault entry instead of the default one, because SMM Profile
// feature depends on IRET instruction to do Single Step
@@ -129,6 +131,11 @@ SmiPFHandler (
DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
);
}
+
+ if (HEAP_GUARD_NONSTOP_MODE) {
+ GuardPagePFHandler (SystemContext.SystemContextIa32->ExceptionData);
+ goto Exit;
+ }
}
CpuDeadLoop ();
}
@@ -146,6 +153,26 @@ SmiPFHandler (
);
CpuDeadLoop ();
}
+
+ //
+ // If NULL pointer was just accessed
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
+ (PFAddress < EFI_PAGE_SIZE)) {
+ DumpCpuContext (InterruptType, SystemContext);
+ DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
+ );
+
+ if (NULL_DETECTION_NONSTOP_MODE) {
+ GuardPagePFHandler (SystemContext.SystemContextIa32->ExceptionData);
+ goto Exit;
+ }
+
+ CpuDeadLoop ();
+ }
+
if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
DumpCpuContext (InterruptType, SystemContext);
DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%x)!\n", PFAddress));
@@ -156,19 +183,6 @@ SmiPFHandler (
}
}
- //
- // If NULL pointer was just accessed
- //
- if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
- (PFAddress < EFI_PAGE_SIZE)) {
- DumpCpuContext (InterruptType, SystemContext);
- DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
- DEBUG_CODE (
- DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextIa32->Eip);
- );
- CpuDeadLoop ();
- }
-
if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
SmmProfilePFHandler (
SystemContext.SystemContextIa32->Eip,
@@ -179,6 +193,7 @@ SmiPFHandler (
SmiDefaultPFHandler ();
}
+Exit:
ReleaseSpinLock (mPFLock);
}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
index fa02c1016c..879fa0ba63 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm
@@ -20,6 +20,7 @@
extern ASM_PFX(FeaturePcdGet (PcdCpuSmmProfileEnable))
extern ASM_PFX(SmiPFHandler)
+extern ASM_PFX(mSetupDebugTrap)
global ASM_PFX(gcSmiIdtr)
global ASM_PFX(gcSmiGdtr)
@@ -673,7 +674,7 @@ o16 mov [ecx + IA32_TSS._SS], ax
mov esp, ebp
; Set single step DB# if SMM profile is enabled and page fault exception happens
- cmp byte [dword ASM_PFX(FeaturePcdGet (PcdCpuSmmProfileEnable))], 0
+ cmp byte [dword ASM_PFX(mSetupDebugTrap)], 0
jz @Done2
; Create return context for iretd in stub function
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index b4fe0bc23b..a743cf64f9 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -51,6 +51,11 @@ BOOLEAN mBtsSupported = TRUE;
//
BOOLEAN mSmmProfileStart = FALSE;
+//
+// The flag indicates if #DB will be setup in #PF handler.
+//
+BOOLEAN mSetupDebugTrap = FALSE;
+
//
// Record the page fault exception count for one instruction execution.
//
@@ -229,7 +234,9 @@ DebugExceptionHandler (
UINTN CpuIndex;
UINTN PFEntry;
- if (!mSmmProfileStart) {
+ if (!mSmmProfileStart &&
+ !HEAP_GUARD_NONSTOP_MODE &&
+ !NULL_DETECTION_NONSTOP_MODE) {
return;
}
CpuIndex = GetCpuIndex ();
@@ -1174,7 +1181,9 @@ InitSmmProfile (
//
// Skip SMM profile initialization if feature is disabled
//
- if (!FeaturePcdGet (PcdCpuSmmProfileEnable)) {
+ if (!FeaturePcdGet (PcdCpuSmmProfileEnable) &&
+ !HEAP_GUARD_NONSTOP_MODE &&
+ !NULL_DETECTION_NONSTOP_MODE) {
return;
}
@@ -1187,6 +1196,11 @@ InitSmmProfile (
// Initialize profile IDT.
//
InitIdtr ();
+
+ //
+ // Tell #PF handler to prepare a #DB subsequently.
+ //
+ mSetupDebugTrap = TRUE;
}
/**
@@ -1294,6 +1308,46 @@ RestorePageTableBelow4G (
}
}
+/**
+ Handler for Page Fault triggered by Guard page.
+
+ @param ErrorCode The Error code of exception.
+
+**/
+VOID
+GuardPagePFHandler (
+ UINTN ErrorCode
+ )
+{
+ UINT64 *PageTable;
+ UINT64 PFAddress;
+ UINT64 RestoreAddress;
+ UINTN RestorePageNumber;
+ UINTN CpuIndex;
+
+ PageTable = (UINT64 *)AsmReadCr3 ();
+ PFAddress = AsmReadCr2 ();
+ CpuIndex = GetCpuIndex ();
+
+ //
+ // Memory operation cross pages, like "rep mov" instruction, will cause
+ // infinite loop between this and Debug Trap handler. We have to make sure
+ // that current page and the page followed are both in PRESENT state.
+ //
+ RestorePageNumber = 2;
+ RestoreAddress = PFAddress;
+ while (RestorePageNumber > 0) {
+ RestorePageTableBelow4G (PageTable, RestoreAddress, CpuIndex, ErrorCode);
+ RestoreAddress += EFI_PAGE_SIZE;
+ RestorePageNumber--;
+ }
+
+ //
+ // Flush TLB
+ //
+ CpuFlushTlb ();
+}
+
/**
The Page fault handler to save SMM profile data.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
index 04a3dfb2e8..c2a48235ab 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
@@ -114,6 +114,17 @@ GetCpuIndex (
VOID
);
+/**
+ Handler for Page Fault triggered by Guard page.
+
+ @param ErrorCode The Error code of exception.
+
+**/
+VOID
+GuardPagePFHandler (
+ UINTN ErrorCode
+ );
+
//
// The flag indicates if execute-disable is supported by processor.
//
@@ -122,5 +133,9 @@ extern BOOLEAN mXdSupported;
// The flag indicates if execute-disable is enabled on processor.
//
extern BOOLEAN mXdEnabled;
+//
+// The flag indicates if #DB will be setup in #PF handler.
+//
+extern BOOLEAN mSetupDebugTrap;
#endif // _SMM_PROFILE_H_
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
index 1613e9cd5c..bacb2f8ad3 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
@@ -64,6 +64,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#define MSR_DEBUG_CTL_BTINT 0x100
#define MSR_DS_AREA 0x600
+#define HEAP_GUARD_NONSTOP_MODE \
+ ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT3|BIT2)) > BIT6)
+
+#define NULL_DETECTION_NONSTOP_MODE \
+ ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT1)) > BIT6)
+
typedef struct {
EFI_PHYSICAL_ADDRESS Base;
EFI_PHYSICAL_ADDRESS Top;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 0fe944fc18..5bb7d57238 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -300,7 +300,9 @@ SmmInitPageTable (
}
}
- if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
+ if (FeaturePcdGet (PcdCpuSmmProfileEnable) ||
+ HEAP_GUARD_NONSTOP_MODE ||
+ NULL_DETECTION_NONSTOP_MODE) {
//
// Set own Page Fault entry instead of the default one, because SMM Profile
// feature depends on IRET instruction to do Single Step
@@ -846,6 +848,11 @@ SmiPFHandler (
DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
);
}
+
+ if (HEAP_GUARD_NONSTOP_MODE) {
+ GuardPagePFHandler (SystemContext.SystemContextX64->ExceptionData);
+ goto Exit;
+ }
}
CpuDeadLoop ();
}
@@ -863,6 +870,26 @@ SmiPFHandler (
);
CpuDeadLoop ();
}
+
+ //
+ // If NULL pointer was just accessed
+ //
+ if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
+ (PFAddress < EFI_PAGE_SIZE)) {
+ DumpCpuContext (InterruptType, SystemContext);
+ DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
+ DEBUG_CODE (
+ DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
+ );
+
+ if (NULL_DETECTION_NONSTOP_MODE) {
+ GuardPagePFHandler (SystemContext.SystemContextX64->ExceptionData);
+ goto Exit;
+ }
+
+ CpuDeadLoop ();
+ }
+
if (IsSmmCommBufferForbiddenAddress (PFAddress)) {
DumpCpuContext (InterruptType, SystemContext);
DEBUG ((DEBUG_ERROR, "Access SMM communication forbidden address (0x%lx)!\n", PFAddress));
@@ -873,19 +900,6 @@ SmiPFHandler (
}
}
- //
- // If NULL pointer was just accessed
- //
- if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT1) != 0 &&
- (PFAddress < EFI_PAGE_SIZE)) {
- DumpCpuContext (InterruptType, SystemContext);
- DEBUG ((DEBUG_ERROR, "!!! NULL pointer access !!!\n"));
- DEBUG_CODE (
- DumpModuleInfoByIp ((UINTN)SystemContext.SystemContextX64->Rip);
- );
- CpuDeadLoop ();
- }
-
if (FeaturePcdGet (PcdCpuSmmProfileEnable)) {
SmmProfilePFHandler (
SystemContext.SystemContextX64->Rip,
@@ -895,6 +909,7 @@ SmiPFHandler (
SmiDefaultPFHandler ();
}
+Exit:
ReleaseSpinLock (mPFLock);
}
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
2018-08-21 3:05 ` [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM Jian J Wang
@ 2018-08-21 15:44 ` Laszlo Ersek
2018-08-22 7:16 ` Wang, Jian J
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-08-21 15:44 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Eric Dong
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> fix GCC build error caused by an unused variable in GuardPagePFHandler()
>
> Since SMM profile feature has already implemented non-stop mode if #PF
> occurred, this patch just makes use of the existing implementation to
> accommodate heap guard and NULL pointer detection feature.
>
> 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/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 43 ++++++++++++------
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 3 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 58 +++++++++++++++++++++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15 ++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 6 +++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 43 ++++++++++++------
> 6 files changed, 137 insertions(+), 31 deletions(-)
It seems like the changes are no-ops for platforms that set all these
PCDs to zero, so from my side
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
2018-08-21 15:44 ` Laszlo Ersek
@ 2018-08-22 7:16 ` Wang, Jian J
0 siblings, 0 replies; 15+ messages in thread
From: Wang, Jian J @ 2018-08-22 7:16 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Dong, Eric
Hi Laszlo,
Thanks.
Regards,
Jian
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, August 21, 2018 11:45 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> fix GCC build error caused by an unused variable in GuardPagePFHandler()
>
> Since SMM profile feature has already implemented non-stop mode if #PF
> occurred, this patch just makes use of the existing implementation to
> accommodate heap guard and NULL pointer detection feature.
>
> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 43 ++++++++++++------
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 3 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 58 +++++++++++++++++++++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15 ++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 6 +++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 43 ++++++++++++------
> 6 files changed, 137 insertions(+), 31 deletions(-)
It seems like the changes are no-ops for platforms that set all these
PCDs to zero, so from my side
Acked-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/4] Support non-stop mode in heap guard and null detection
2018-08-21 3:05 [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Jian J Wang
` (3 preceding siblings ...)
2018-08-21 3:05 ` [PATCH v2 4/4] UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM Jian J Wang
@ 2018-08-21 14:28 ` Laszlo Ersek
2018-08-22 6:59 ` Wang, Jian J
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-08-21 14:28 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> fix GCC build error
>
> Background:
> Heap Guard and NULL Pointer Detection are very useful features to detect
> code flaw in EDK II. If an issue is detected, #PF exception will be
> triggered and the BIOS will enter into dead loop, which is the default
> behavior of exception handling. From QA perspective, this default behavior
> will block them to collect all tests result in reasonable time.
>
> Solution:
> This patch series update CpuDxe, PiSmmCpuDxeSmm and CpuExceptionHandlerLib
> to allow the code to continue execution after #PF. The mechanism behind it
> is the same as SMM Profile feature, in which a special #PF handler is
> registered to set the page causing #PF to be 'present' and setup single
> steop trap, then return the control back to the instruction accessing that
> page. Once the instruction is re-executed, a #DB is triggered and a special
> handler for it will be called to reset the page back to 'not-present'.
>
> Usage:
> The non-stop mode is enabled/disabled by BIT6 of following PCDs
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>
> The default setting is 'disable'.
>
> BZ Tracker:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1095
>
> OS Boot Validation:
> Platform: OVMF
> OS (x64): Fedora 26, Ubuntu 18.04, Windows 10, Windows 7
>
> Jian J Wang (4):
> MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs
> UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
> UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
> UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
>
> MdeModulePkg/MdeModulePkg.dec | 4 +-
> UefiCpuPkg/CpuDxe/CpuDxe.h | 39 +++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> UefiCpuPkg/CpuDxe/CpuMp.c | 34 ++-
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 271 +++++++++++++++++++++
> .../Ia32/ExceptionHandlerAsm.nasm | 7 +
> .../Ia32/ExceptionTssEntryAsm.nasm | 4 +-
> .../X64/ExceptionHandlerAsm.nasm | 4 +
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 43 ++--
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 3 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 58 ++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 6 +
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 43 ++--
> 14 files changed, 493 insertions(+), 41 deletions(-)
>
(1) This looks like a feature addition, so please include the BZ
reference (1095) on the following wiki page:
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
(under "Proposed Features")
(2) The general description should be moved (or copied) from this email
(v2 0/4) to patch #1 (v2 1/4). The cover letter is not captured in the
commit log, and I think there isn't going to be any other documentation
for the feature than the DEC file.
(Note that I'm not suggesting that you add the documentation to the DEC
file in patch #1 -- instead, the commit message on patch #1 should
contain it. Then people can find the commit from the DEC file with "git
blame", and read the description.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/4] Support non-stop mode in heap guard and null detection
2018-08-21 14:28 ` [PATCH v2 0/4] Support non-stop mode in heap guard and null detection Laszlo Ersek
@ 2018-08-22 6:59 ` Wang, Jian J
0 siblings, 0 replies; 15+ messages in thread
From: Wang, Jian J @ 2018-08-22 6:59 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Hi Laszlo,
Regards,
Jian
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, August 21, 2018 10:28 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v2 0/4] Support non-stop mode in heap guard and null detection
On 08/21/18 05:05, Jian J Wang wrote:
>> v2 changes:
>> fix GCC build error
>
> Background:
> Heap Guard and NULL Pointer Detection are very useful features to detect
> code flaw in EDK II. If an issue is detected, #PF exception will be
> triggered and the BIOS will enter into dead loop, which is the default
> behavior of exception handling. From QA perspective, this default behavior
> will block them to collect all tests result in reasonable time.
>
> Solution:
> This patch series update CpuDxe, PiSmmCpuDxeSmm and CpuExceptionHandlerLib
> to allow the code to continue execution after #PF. The mechanism behind it
> is the same as SMM Profile feature, in which a special #PF handler is
> registered to set the page causing #PF to be 'present' and setup single
> steop trap, then return the control back to the instruction accessing that
> page. Once the instruction is re-executed, a #DB is triggered and a special
> handler for it will be called to reset the page back to 'not-present'.
>
> Usage:
> The non-stop mode is enabled/disabled by BIT6 of following PCDs
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
>
> The default setting is 'disable'.
>
> BZ Tracker:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1095
>
> OS Boot Validation:
> Platform: OVMF
> OS (x64): Fedora 26, Ubuntu 18.04, Windows 10, Windows 7
>
> Jian J Wang (4):
> MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs
> UefiCpuPkg/CpuExceptionHandlerLib: Setup single step in #PF handler
> UefiCpuPkg/CpuDxe: implement non-stop mode for uefi
> UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for SMM
>
> MdeModulePkg/MdeModulePkg.dec | 4 +-
> UefiCpuPkg/CpuDxe/CpuDxe.h | 39 +++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> UefiCpuPkg/CpuDxe/CpuMp.c | 34 ++-
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 271 +++++++++++++++++++++
> .../Ia32/ExceptionHandlerAsm.nasm | 7 +
> .../Ia32/ExceptionTssEntryAsm.nasm | 4 +-
> .../X64/ExceptionHandlerAsm.nasm | 4 +
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 43 ++--
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 3 +-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 58 ++++-
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h | 15 ++
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h | 6 +
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 43 ++--
> 14 files changed, 493 insertions(+), 41 deletions(-)
>
(1) This looks like a feature addition, so please include the BZ
reference (1095) on the following wiki page:
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
(under "Proposed Features")
[Jian] Sure. I’ll add it.
(2) The general description should be moved (or copied) from this email
(v2 0/4) to patch #1 (v2 1/4). The cover letter is not captured in the
commit log, and I think there isn't going to be any other documentation
for the feature than the DEC file.
(Note that I'm not suggesting that you add the documentation to the DEC
file in patch #1 -- instead, the commit message on patch #1 should
contain it. Then people can find the commit from the DEC file with "git
blame", and read the description.)
[Jian] You’re right. I forgot this point. I’ll copy it. Thanks.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread