Re: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers

[Yao, Jiewen]

Some suggestion:

1) Would you please use meaning definition for BIT2?

if ((SegmentSelector & BIT2) == 0) {

2) Can we just use (SegmentSelector & ~0x7) for below?

((SegmentSelector >> 3) * 8)

3) Below calculation seems wrong. Should it be: SegDescLimitInBytes = 
(UINTN)SegDescLimit * SIZE_4KB + (SIZE_4KB - 1) ?

  if (SegmentDescriptor->Bits.G == 1) {
SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;

Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Friday, December 29, 2017 12:40 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <ler...@redhat.com>; Dong, Eric <eric.d...@intel.com>
> Subject: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid
> frame/stack pointers
> 
> Validate all possible memory dereferences during stack traces in IA32
> and X64 CPU exceptions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Requested-by: Brian Johnson <brian.john...@hpe.com>
> Requested-by: Jiewen Yao <jiewen@intel.com>
> Signed-off-by: Paulo Alcantara <pa...@paulo.ac>
> ---
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c |
> 143 +++-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |
> 75 +-
>  2 files changed, 210 insertions(+), 8 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> index 25e02fbbc1..9b52d4f6d2 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> @@ -398,6 +398,96 @@ DumpCpuContext (
>  );
>  }
> 
> +/**
> +  Check if a logical address is valid.
> +
> +  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
> +  @param[in]  SegmentSelectorSegment selector.
> +  @param[in]  Offset Offset or logical address.
> +**/
> +STATIC
> +BOOLEAN
> +IsLogicalAddressValid (
> +  IN  EFI_SYSTEM_CONTEXT   SystemContext,
> +  IN  UINT16   SegmentSelector,
> +  IN  UINTNOffset
> +  )
> +{
> +  IA32_SEGMENT_DESCRIPTOR  *SegmentDescriptor;
> +  UINT32   SegDescBase;
> +  UINT32   SegDescLimit;
> +  UINTNSegDescLimitInBytes;
> +
> +  //
> +  // Check for valid input parameters
> +  //
> +  if (SegmentSelector == 0 || Offset == 0) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Check whether to look for a segment descriptor in GDT or LDT table
> +  //
> +  if ((SegmentSelector & BIT2) == 0) {
> +//
> +// Get segment descriptor from GDT table
> +//
> +SegmentDescriptor =
> +  (IA32_SEGMENT_DESCRIPTOR *)(
> +(UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
> +((SegmentSelector >> 3) * 8)
> +);
> +  } else {
> +//
> +// Get segment descriptor from LDT table
> +//
> +SegmentDescriptor =
> +  (IA32_SEGMENT_DESCRIPTOR *)(
> +(UINTN)SystemContext.SystemContextIa32->Ldtr +
> +((SegmentSelector >> 3) * 8)
> +);
> +  }
> +
> +  //
> +  // Get segment descriptor's base address
> +  //
> +  SegDescBase = SegmentDescriptor->Bits.BaseLow |
> +(SegmentDescriptor->Bits.BaseMid << 16) |
> +(SegmentDescriptor->Bits.BaseHigh << 24);
> +
> +  //
> +  // Get segment descriptor's limit
> +  //
> +  SegDescLimit = SegmentDescriptor->Bits.LimitLow |
> +(SegmentDescriptor->Bits.LimitHigh << 16);
> +
> +  //
> +  // Calculate segment descriptor's limit in bytes
> +  //
> +  if (SegmentDescriptor->Bits.G == 1) {
> +SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;
> +  } else {
> +SegDescLimitInBytes = SegDescLimit;
> +  }
> +
> +  //
> +  // Make sure to not access beyond a segment limit boundary
> +  //
> +  if (Offset + SegDescBase > SegDescLimitInBytes) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Check if the translated logical address (or linear address) is valid
> +  //
> +  return IsLinearAddressValid (
> +SystemContext.SystemContextIa32->Cr0,
> +SystemContext.SystemContextIa32->Cr3,
> +SystemContext.SystemContextIa32->Cr4,
> +Offset + SegDescBase
> +);
> +}
> +
>  /**
>Dump stack trace.
> 
> @@ -459,6 +549,20 @@ DumpStackTrace (
>Interna

[edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers

[Paulo Alcantara]

Validate all possible memory dereferences during stack traces in IA32
and X64 CPU exceptions.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 143 
+++-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |  75 
+-
 2 files changed, 210 insertions(+), 8 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 25e02fbbc1..9b52d4f6d2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -398,6 +398,96 @@ DumpCpuContext (
 );
 }
 
+/**
+  Check if a logical address is valid.
+
+  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
+  @param[in]  SegmentSelectorSegment selector.
+  @param[in]  Offset Offset or logical address.
+**/
+STATIC
+BOOLEAN
+IsLogicalAddressValid (
+  IN  EFI_SYSTEM_CONTEXT   SystemContext,
+  IN  UINT16   SegmentSelector,
+  IN  UINTNOffset
+  )
+{
+  IA32_SEGMENT_DESCRIPTOR  *SegmentDescriptor;
+  UINT32   SegDescBase;
+  UINT32   SegDescLimit;
+  UINTNSegDescLimitInBytes;
+
+  //
+  // Check for valid input parameters
+  //
+  if (SegmentSelector == 0 || Offset == 0) {
+return FALSE;
+  }
+
+  //
+  // Check whether to look for a segment descriptor in GDT or LDT table
+  //
+  if ((SegmentSelector & BIT2) == 0) {
+//
+// Get segment descriptor from GDT table
+//
+SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+(UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
+((SegmentSelector >> 3) * 8)
+);
+  } else {
+//
+// Get segment descriptor from LDT table
+//
+SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+(UINTN)SystemContext.SystemContextIa32->Ldtr +
+((SegmentSelector >> 3) * 8)
+);
+  }
+
+  //
+  // Get segment descriptor's base address
+  //
+  SegDescBase = SegmentDescriptor->Bits.BaseLow |
+(SegmentDescriptor->Bits.BaseMid << 16) |
+(SegmentDescriptor->Bits.BaseHigh << 24);
+
+  //
+  // Get segment descriptor's limit
+  //
+  SegDescLimit = SegmentDescriptor->Bits.LimitLow |
+(SegmentDescriptor->Bits.LimitHigh << 16);
+
+  //
+  // Calculate segment descriptor's limit in bytes
+  //
+  if (SegmentDescriptor->Bits.G == 1) {
+SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;
+  } else {
+SegDescLimitInBytes = SegDescLimit;
+  }
+
+  //
+  // Make sure to not access beyond a segment limit boundary
+  //
+  if (Offset + SegDescBase > SegDescLimitInBytes) {
+return FALSE;
+  }
+
+  //
+  // Check if the translated logical address (or linear address) is valid
+  //
+  return IsLinearAddressValid (
+SystemContext.SystemContextIa32->Cr0,
+SystemContext.SystemContextIa32->Cr3,
+SystemContext.SystemContextIa32->Cr4,
+Offset + SegDescBase
+);
+}
+
 /**
   Dump stack trace.
 
@@ -459,6 +549,20 @@ DumpStackTrace (
   InternalPrintMessage ("\nCall trace:\n");
 
   for (;;) {
+//
+// Check for valid frame pointer
+//
+if (!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp + 4) ||
+!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp)) {
+  InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+"pointer at 0x%08x\n", __FUNCTION__, Ebp);
+  break;
+}
+
 //
 // Print stack frame in the following format:
 //
@@ -588,6 +692,16 @@ DumpImageModuleNames (
   // Walk through call stack and find next module names
   //
   for (;;) {
+if (!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp) ||
+!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp + 4)) {
+  InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+"pointer at 0x%08x\n", __FUNCTION__, Ebp);
+}
+
 //
 // Set EIP with return address from current stack frame
 //
@@ -651,16 +765,23 @@ DumpImageModuleNames (
 /**
   Dump stack contents.
 
-  @param[in]  CurrentEsp Current stack pointer address.
+  @param[in]  SystemContext   Pointer to