5) For CR4, please use meaning definition for BIT4/BIT5. if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) { if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFFFFFFFFFULL) ||
6) For IA32 PAE/PSE calculation, same comment for 3 and 4. 7) Last but not least important, would you please share the information on how do you validate the 32bit PAE/PSE/normal 4K page table? Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao, > Jiewen > Sent: Thursday, January 4, 2018 9:36 AM > To: Paulo Alcantara <pa...@paulo.ac>; edk2-devel@lists.01.org > Cc: Laszlo Ersek <ler...@redhat.com>; Dong, Eric <eric.d...@intel.com> > Subject: Re: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add > helper to valid memory addresses > > 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-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 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.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/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 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel