Re: [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

["Yao, Jiewen" <jiewen.yao@intel.com>]

Some suggestion:

1) I am not sure if it is proper to use ASSERT in an exception handler, because we know something is wrong.

  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);

I suggest we just do the check, and return FALSE, if the prerequisite is not satisfied.

2) Can we use meaningful definition for BIT0, BIT7?

  if ((*Pml4TableEntry & BIT0) == 0) {
  if ((*PageDirPtrTableEntry & BIT7) != 0) {

3) I am not sure if I understand below code.

  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);

If MaxPhyAddrBits is 48, you will get "Cr3 & 0x0FFFFFFFFFFFF000". Is that what you want? I think we need "Cr3 & 0x0000FFFFFFFFF000"
Should it be: PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) & (~0xFFF);

4) Can we use a more readable way to below? Personally, I do not suggest "<< 3", which is just the index calculation.

  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);

For example:
  PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) & (~0xFFF);
  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
  Index= (UINTN)(((UINT64)LinearAddress >> 39) & 0x1FF);
  PhysicalAddress = Pml4TableEntry[Index] & ((1ULL << MaxPhyAddrBits) - 1) & (~0xFFF);



Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@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 <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to
> valid memory addresses
> 
> Introduce IsLinearAddressValid() function that will be used for
> validating memory addresses that would get dereferenced during stack
> traces in IA32 and X64 CPU exceptions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Requested-by: Brian Johnson <brian.johnson@hpe.com>
> Requested-by: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Paulo Alcantara <paulo@paulo.ac>
> ---
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 382
> ++++++++++++++++++++
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h |  16 +
>  2 files changed, 398 insertions(+)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> index 867c5c01d6..52b3eb1463 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> @@ -14,6 +14,9 @@
> 
>  #include "CpuExceptionCommon.h"
> 
> +#include <Register/Msr.h>
> +#include <Library/DebugLib.h>
> +
>  //
>  // Error code flag indicating whether or not an error code will be
>  // pushed on the stack if an exception occurs.
> @@ -194,3 +197,382 @@ GetPdbFileName (
>      }
>    }
>  }
> +
> +/**
> +  Check if a linear address is valid by walking the page tables in 4-level
> +  paging mode.
> +
> +  @param[in]  Cr3             CR3 control register.
> +  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
> +  @param[in]  LinearAddress   Linear address to be checked.
> +**/
> +STATIC
> +BOOLEAN
> +Do4LevelPagingModeCheck (
> +  IN UINTN            Cr3,
> +  IN UINT8            MaxPhyAddrBits,
> +  IN UINTN            LinearAddress
> +  )
> +{
> +  UINT64 PhysicalAddress;
> +  UINT64 *Pml4TableEntry;
> +  UINT64 *PageDirPtrTableEntry;
> +  UINT64 *PageDirEntry;
> +  UINT64 *PageTableEntry;
> +
> +  //
> +  // In 4-level paging mode, linear addresses are 48 bits wide
> +  //
> +  if ((UINT64)LinearAddress > (1ULL << 48) - 1) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PML4E
> +  //
> +  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);
> +
> +  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if a PDPTE is present
> +  //
> +  if ((*Pml4TableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PDPTE
> +  //
> +  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) <<
> 12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 30) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageDirPtrTableEntry) - 1)) == 0);
> +
> +  PageDirPtrTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check whether a PDPTE or 1GiB page entry is present
> +  //
> +  if ((*PageDirPtrTableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check if PDPTE maps an 1GiB page
> +  //
> +  if ((*PageDirPtrTableEntry & BIT7) != 0) {
> +    return TRUE;
> +  }
> +
> +  //
> +  // Calculate physical address of PDE
> +  //
> +  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1)
> <<
> +                                             12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 21) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageDirEntry) - 1)) == 0);
> +
> +  PageDirEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check whether a PDE or a 2MiB page entry is present
> +  //
> +  if ((*PageDirEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check if PDE maps a 2MiB page
> +  //
> +  if ((*PageDirEntry & BIT7) != 0) {
> +    return TRUE;
> +  }
> +
> +  //
> +  // Calculate physical address of PTE
> +  //
> +  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 12) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageTableEntry) - 1)) == 0);
> +
> +  PageTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if PTE maps a 4KiB page
> +  //
> +  if ((*PageTableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  Check if a linear address is valid by walking the page tables in 32-bit paging
> +  mode.
> +
> +  @param[in]  Cr3             CR3 control register.
> +  @param[in]  Cr4             CR4 control register.
> +  @param[in]  LinearAddress   Linear address to be checked.
> +**/
> +STATIC
> +BOOLEAN
> +Do32BitPagingModeCheck (
> +  IN UINTN            Cr3,
> +  IN UINTN            Cr4,
> +  IN UINTN            LinearAddress
> +  )
> +{
> +  UINT64 PhysicalAddress;
> +  UINT32 *PageDirEntry;
> +  UINT32 *PageTableEntry;
> +
> +  if (LinearAddress > MAX_UINT32) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PDE
> +  //
> +  PhysicalAddress = (UINT32)Cr3 & (((1ULL << 20) - 1) << 12);
> +  PhysicalAddress |= (((UINT32)LinearAddress >> 22) & 0x3FF) << 2;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageDirEntry) - 1)) == 0);
> +
> +  PageDirEntry = (UINT32 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check whether a PTE or a 4MiB page is present
> +  //
> +  if ((*PageDirEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check if PDE maps a 4MiB page
> +  //
> +  if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
> +    return TRUE;
> +  }
> +
> +  //
> +  // Calculate physical address of PTE
> +  //
> +  PhysicalAddress = *PageDirEntry & (((1ULL << 20) - 1) << 12);
> +  PhysicalAddress |= (((UINT32)LinearAddress >> 12) & 0x3FF) << 2;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageTableEntry) - 1)) == 0);
> +
> +  PageTableEntry = (UINT32 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if PTE maps a 4KiB page
> +  //
> +  if ((*PageTableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  Check if a linear address is valid by walking the page tables in PAE paging
> +  mode.
> +
> +  @param[in]  Cr3             CR3 control register.
> +  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
> +  @param[in]  LinearAddress   Linear address to be checked.
> +**/
> +STATIC
> +BOOLEAN
> +DoPAEPagingModeCheck (
> +  IN UINTN            Cr3,
> +  IN UINT8            MaxPhyAddrBits,
> +  IN UINTN            LinearAddress
> +  )
> +{
> +  UINT64 PhysicalAddress;
> +  UINT64 *PageDirPtrTableEntry;
> +  UINT64 *PageDirEntry;
> +  UINT64 *PageTableEntry;
> +
> +  if (LinearAddress > MAX_UINT32) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PDPTE
> +  //
> +  PhysicalAddress = (UINT32)Cr3 >> 5;
> +
> +  //
> +  // Select PDPTE register
> +  //
> +  PhysicalAddress +=
> +    ((UINT32)LinearAddress >> 30) * sizeof (*PageDirPtrTableEntry);
> +
> +  PageDirPtrTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if PDE is present
> +  //
> +  if ((*PageDirPtrTableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1)
> <<
> +                                             12);
> +  PhysicalAddress |= ((LinearAddress >> 21) & 0x1FF) << 3;
> +  ASSERT ((PhysicalAddress & (sizeof (*PageDirEntry) - 1)) == 0);
> +
> +  PageDirEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check whether a PTE or a 2MiB page is present
> +  //
> +  if ((*PageDirEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check if PDE maps a 2MiB page
> +  //
> +  if ((*PageDirEntry & BIT7) != 0) {
> +    return TRUE;
> +  }
> +
> +  //
> +  // Calculate physical address of PTE
> +  //
> +  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> +  PhysicalAddress |= ((LinearAddress >> 12) & 0x1FF) << 3;
> +  ASSERT ((PhysicalAddress & (sizeof (*PageTableEntry) - 1)) == 0);
> +
> +  PageTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if PTE maps a 4KiB page
> +  //
> +  if ((*PageTableEntry & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
> +/**
> +  Check if a linear address is valid.
> +
> +  @param[in]  Cr0            CR0 control register.
> +  @param[in]  Cr3            CR3 control register.
> +  @param[in]  Cr4            CR4 control register.
> +  @param[in]  LinearAddress  Linear address to be checked.
> +**/
> +BOOLEAN
> +IsLinearAddressValid (
> +  IN  UINTN              Cr0,
> +  IN  UINTN              Cr3,
> +  IN  UINTN              Cr4,
> +  IN  UINTN              LinearAddress
> +  )
> +{
> +  UINT32                  Eax;
> +  UINT32                  Edx;
> +  UINT8                   MaxPhyAddrBits;
> +  MSR_IA32_EFER_REGISTER  Msr;
> +  BOOLEAN                 AddressValid;
> +
> +  //
> +  // Check for valid input parameters
> +  //
> +  if (Cr0 == 0 || Cr4 == 0 || LinearAddress == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check if paging is disabled
> +  //
> +  if ((Cr0 & BIT31) == 0) {
> +    //
> +    // If CR4.PAE bit is set, then the linear (or physical) address supports
> +    // only up to 36 bits.
> +    //
> +    if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFFFFFFFFFULL) ||
> +        LinearAddress > 0xFFFFFFFF) {
> +      return FALSE;
> +    }
> +
> +    return TRUE;
> +  }
> +
> +  //
> +  // Paging can be enabled only if CR0.PE bit is set
> +  //
> +  if ((Cr0 & BIT0) == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // CR3 register cannot be zero if paging is enabled
> +  //
> +  if (Cr3 == 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Get MAXPHYADDR bits
> +  //
> +  AsmCpuid (0x80000000, &Eax, NULL, NULL, NULL);
> +  if (Eax >= 0x80000008) {
> +    AsmCpuid (0x80000008, &Eax, NULL, NULL, NULL);
> +    MaxPhyAddrBits = (UINT8)Eax;
> +  } else {
> +    AsmCpuid (1, NULL, NULL, NULL, &Edx);
> +    if ((Edx & BIT6) != 0) {
> +      MaxPhyAddrBits = 36;
> +    } else {
> +      MaxPhyAddrBits = 32;
> +    }
> +  }
> +
> +  ASSERT (MaxPhyAddrBits > 0);
> +
> +  AddressValid = FALSE;
> +
> +  //
> +  // check if CR4.PAE bit is not set
> +  //
> +  if ((Cr4 & BIT5) == 0) {
> +    //
> +    // Check if linear address is valid in 32-bit paging mode
> +    //
> +    AddressValid = Do32BitPagingModeCheck (Cr3, Cr4, LinearAddress);
> +  } else {
> +    if (MaxPhyAddrBits > 52) {
> +      return FALSE;
> +    }
> +
> +    Msr.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
> +
> +    if (Msr.Bits.LME == 0) {
> +      //
> +      // Check if linear address is valid in PAE paging mode
> +      //
> +      AddressValid = DoPAEPagingModeCheck (Cr3, MaxPhyAddrBits,
> LinearAddress);
> +    } else {
> +      //
> +      // Check if linear address is valid in 4-level paging mode
> +      //
> +      AddressValid = Do4LevelPagingModeCheck (Cr3, MaxPhyAddrBits,
> +                                              LinearAddress);
> +    }
> +  }
> +
> +  return AddressValid;
> +}
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> index ec46c2d9d3..1b51034c25 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> @@ -330,5 +330,21 @@ GetPdbFileName (
>    OUT CHAR8    **PdbFileName
>    );
> 
> +/**
> +  Check if a linear address is valid.
> +
> +  @param[in]  Cr0            CR0 control register.
> +  @param[in]  Cr3            CR3 control register.
> +  @param[in]  Cr4            CR4 control register.
> +  @param[in]  LinearAddress  Linear address to be checked.
> +**/
> +BOOLEAN
> +IsLinearAddressValid (
> +  IN  UINTN              Cr0,
> +  IN  UINTN              Cr3,
> +  IN  UINTN              Cr4,
> +  IN  UINTN              LinearAddress
> +  );
> +
>  #endif
> 
> --
> 2.14.3
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel