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

[Paulo Alcantara]

"Yao, Jiewen" <jiewen@intel.com> writes:

Hi Jiewen,

> For 7), you are right that we disable IA32 paging by default.
> However, we do support IA32 PAE in DxeIpl. Please refer to 
> MdeModulePkg\Core\DxeIplPeim\Ia32\DxeLoadFunc.c
> ==
> PageTables = 0;
> BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && 
> IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
> if (BuildPageTablesIa32Pae) {
>   PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
>   EnableExecuteDisableBit ();
> }
> ==
>
> Please notice that we only support IA32 PAE, we do not support IA32
> non-PAE mode so far. (no matter PSE is ON/OFF)

Cool! I didn't know about it.

>
> So, I suggest:
> 7.1) Please validate IA32 PAE mode. (You can enable PcdSetNxForStack)

I will. Thanks!

> 7.2) If we cannot validate the IA32 non-PAE code, please remove
> them. You can just print "IA32 non-PAE - UNSUPPORTED" and return
> invalid address. Then it can save our development time, review time,
> and validation time. The key is that we only want to check in the
> validated code.

You're right. I'll keep only the code that we were able to validate its
implementation (e.g. 4-level & 32-bit PAE).

Thanks!
Paulo

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
>> Alcantara
>> Sent: Thursday, January 4, 2018 9:30 PM
>> To: Yao, Jiewen <jiewen@intel.com>; Yao, Jiewen <jiewen@intel.com>;
>> 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
>> 
>> "Yao, Jiewen" <jiewen@intel.com> writes:
>> 
>> > 5) For CR4, please use meaning definition for BIT4/BIT5.
>> >   if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
>> > if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||
>> 
>> OK.
>> 
>> >
>> > 6) For IA32 PAE/PSE calculation, same comment for 3 and 4.
>> 
>> OK.
>> 
>> >
>> > 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?
>> 
>> Since on IA32 we use 32-bit protected flat model and paging disabled
>> (OK?), I wasn't able to validate the paging modes other than 4-level
>> paging mode in X64. The memory validation code I wrote is heavily based
>> upon what I read from Intel SDM Vol 3A manual.
>> 
>> If you do have any idea on how to validate it -- whether it's a PoC or
>> test code -- please let me know, and then I validate it.
>> 
>> Thanks again for your review!
>> 
>> Paulo
>> 
>> >
>> > 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)
>> >&

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

[Yao, Jiewen]

Thank you!

For 7), you are right that we disable IA32 paging by default.
However, we do support IA32 PAE in DxeIpl. Please refer to 
MdeModulePkg\Core\DxeIplPeim\Ia32\DxeLoadFunc.c
==
PageTables = 0;
BuildPageTablesIa32Pae = (BOOLEAN) (PcdGetBool (PcdSetNxForStack) && 
IsIa32PaeSupport () && IsExecuteDisableBitAvailable ());
if (BuildPageTablesIa32Pae) {
  PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE);
  EnableExecuteDisableBit ();
}
==

Please notice that we only support IA32 PAE, we do not support IA32 non-PAE 
mode so far. (no matter PSE is ON/OFF)

So, I suggest:
7.1) Please validate IA32 PAE mode. (You can enable PcdSetNxForStack)
7.2) If we cannot validate the IA32 non-PAE code, please remove them. You can 
just print "IA32 non-PAE - UNSUPPORTED" and return invalid address. Then it can 
save our development time, review time, and validation time. The key is that we 
only want to check in the validated code.

Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Thursday, January 4, 2018 9:30 PM
> To: Yao, Jiewen <jiewen@intel.com>; Yao, Jiewen <jiewen@intel.com>;
> 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
> 
> "Yao, Jiewen" <jiewen@intel.com> writes:
> 
> > 5) For CR4, please use meaning definition for BIT4/BIT5.
> >   if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
> > if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||
> 
> OK.
> 
> >
> > 6) For IA32 PAE/PSE calculation, same comment for 3 and 4.
> 
> OK.
> 
> >
> > 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?
> 
> Since on IA32 we use 32-bit protected flat model and paging disabled
> (OK?), I wasn't able to validate the paging modes other than 4-level
> paging mode in X64. The memory validation code I wrote is heavily based
> upon what I read from Intel SDM Vol 3A manual.
> 
> If you do have any idea on how to validate it -- whether it's a PoC or
> test code -- please let me know, and then I validate it.
> 
> Thanks again for your review!
> 
> Paulo
> 
> >
> > 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 & 0x0000". Is that
> what
> >> you want? I think we need "Cr3 & 0xF000"
> >> 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 cal

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

[Paulo Alcantara]

"Yao, Jiewen" <jiewen@intel.com> writes:

> 5) For CR4, please use meaning definition for BIT4/BIT5.
>   if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
> if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||

OK.

>
> 6) For IA32 PAE/PSE calculation, same comment for 3 and 4.

OK.

>
> 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?

Since on IA32 we use 32-bit protected flat model and paging disabled
(OK?), I wasn't able to validate the paging modes other than 4-level
paging mode in X64. The memory validation code I wrote is heavily based
upon what I read from Intel SDM Vol 3A manual.

If you do have any idea on how to validate it -- whether it's a PoC or
test code -- please let me know, and then I validate it.

Thanks again for your review!

Paulo

>
> 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 & 0x0000". Is that 
>> what
>> you want? I think we need "Cr3 & 0xF000"
>> 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>
>> > Si

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

[Paulo Alcantara]

"Yao, Jiewen" <jiewen@intel.com> writes:

Hi Jiewen,

> 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.

OK.

>
> 2) Can we use meaningful definition for BIT0, BIT7?
>
>   if ((*Pml4TableEntry & BIT0) == 0) {
>   if ((*PageDirPtrTableEntry & BIT7) != 0) {

Sure. Sorry for missing that.

>
> 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 & 0x0000". Is that 
> what you want? I think we need "Cr3 & 0xF000"
> Should it be: PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) 
> & (~0xFFF);

Yes, it should. I've just re-checked the Intel SDM Vol 3A and your
calculation is correct. Thanks! I'll fix that up in next series.

>
> 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);

Sure. This way is much clearer, indeed.

Thank you very much for the effort on reviewing this!

Paulo

>> -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 
>> +#include 
>> +
>>  //
>>  // 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 UINTNCr3,
>> +  IN U

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

[Paulo Alcantara]

"Brian J. Johnson"  writes:

Hi Brian,

> On 12/28/2017 10:39 PM, Paulo Alcantara wrote:
>> +  //
>> +  // 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 > 0xFULL) ||
>> +LinearAddress > 0x) {
>> +  return FALSE;
>> +}
>> +
>> +return TRUE;
>> +  }
>
> Paulo,
>
> The logic there doesn't look quite right:  if LinearAddress is between 
> 2^32 and 2^36-1, this code will always return FALSE, even if CR4.PAE is 
> set.  Shouldn't it be:
>
> if ((UINT64)LinearAddress > 0xFULL ||
> ((Cr4 & BIT5) == 0 && LinearAddress > 0x)) {
>   return FALSE;
> }

You're right. The check is bogus and I'll fix it up in the next version.

>
> (I haven't examined all the code in detail, I just happened to notice 
> this issue.)

No problem. Your comments are very appreciable.

> This bug should get fixed before pushing this series.  I also have some 
> more general design questions, which shouldn't hold up pushing the 
> series, but I think merit some discussion:
>
> This is great code for validating addresses in general, especially when 
> guard pages are in use for NULL pointers, stack overflow, etc.  Thanks 
> for adding it!  But for [er]sp and [er]bp validation, don't you really 
> just want to know if the address is in the expected stack range?  Maybe 
> the code which sets up the stack could communicate the valid range to 
> CpuExceptionHandlerLib somehow.  It could use special debug register 
> values like 
> SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLib.c 
> does.  Or perhaps it could use dynamic PCDs (although I don't know that 
> it's a good idea to go looking up PCDs in an exception handler.)  Or 
> maybe there's a more straightforward way  It would have to take AP 
> stacks into account, and probably SetJump/LongJump as well.  That may or 
> may not be simpler than the current code

I'm not quite sure if I understood you correctly when you say that I
should be checking whether the address is in the expected range, but
using the debug registers to save call stack information seems like a
good idea, although it doesn't seem to be that simple as you mentioned.

>
> More generally, I'd like to see some sort of platform-specific callout 
> to further validate addresses.  Not all mapped addresses, or addresses 
> up to the architectural limit, are safe to access.  For instance, reads 
> to SMRAM outside of SMM will cause exceptions.  Also, we wouldn't want 
> to go backtracing through MMIO or MMCFG space:  reads there could 
> potentially have side effects on the hardware.

Yes - we should ensure that those regions are not accessed during the
stacktrace, as well as test this implementation a lot more.

>
> The rules can also vary at different points in boot.  For example, 
> before memory is initialized, Intel Xeon processors generally execute 
> 32-bit code in cache-as-RAM mode, where the caches are jury-rigged to 
> operate as temporary storage while the memory setup code is running.  In 
> CAR mode, only a few address ranges can be accessed without causing 
> machine checks:  the cache-as-RAM range containing the stack, heap, and 
> HOB list, the architectural firmware range below 4G, and a few specific 
> MMCFG and MMIO ranges.

Really great info. Thanks. We should take that into account as well.

>
> So I'd like to suggest that you define an AddressValidationLib library 
> class, which provides a routine which takes an address (or an address 
> range?) and an indication of the intended use (memory read, memory 
> write, execute/disassemble code, stack dump, IO, ...), and returns a 
> value specifying if the access is:
> - safe (IsLinearAddressValid() should return TRUE)
> - unsafe (IsLinearAddressValid() should return FALSE)
> - unknown (IsLinearAddressValid() should perform its other tests)
>
> You can supply a NULL instance which always returns "unknown" for 
> platforms which don't want to perform their own validation.

Great idea! I can do that for sure, but first of all, I'd like to make
sure the memory validation code is working correctly for IA32 and X64
platforms before introducing the AddressValidationLib library.

Thank you very much for the review and comments!

Paulo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Yao, Jiewen]

5) For CR4, please use meaning definition for BIT4/BIT5.
  if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||

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 & 0x0000". Is that what
> you want? I think we need "Cr3 & 0xF000"
> 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 
> > +#include 
> > +
> >  //
> >  // Error code flag indicating whether or not an

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

[Yao, Jiewen]

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 & 0x0000". Is that what 
you want? I think we need "Cr3 & 0xF000"
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 
> +#include 
> +
>  //
>  // 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 UINTNCr3,
> +  IN UINT8MaxPhyAddrBits,
> +  IN UINTNLinearAddress
> +  )
> +{
> +  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

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

[Brian J. Johnson]

On 12/28/2017 10:39 PM, Paulo Alcantara wrote:

+  //
+  // 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 > 0xFULL) ||
+LinearAddress > 0x) {
+  return FALSE;
+}
+
+return TRUE;
+  }


Paulo,

The logic there doesn't look quite right:  if LinearAddress is between 
2^32 and 2^36-1, this code will always return FALSE, even if CR4.PAE is 
set.  Shouldn't it be:


   if ((UINT64)LinearAddress > 0xFULL ||
   ((Cr4 & BIT5) == 0 && LinearAddress > 0x)) {
 return FALSE;
   }

(I haven't examined all the code in detail, I just happened to notice 
this issue.)


This bug should get fixed before pushing this series.  I also have some 
more general design questions, which shouldn't hold up pushing the 
series, but I think merit some discussion:


This is great code for validating addresses in general, especially when 
guard pages are in use for NULL pointers, stack overflow, etc.  Thanks 
for adding it!  But for [er]sp and [er]bp validation, don't you really 
just want to know if the address is in the expected stack range?  Maybe 
the code which sets up the stack could communicate the valid range to 
CpuExceptionHandlerLib somehow.  It could use special debug register 
values like 
SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLib.c 
does.  Or perhaps it could use dynamic PCDs (although I don't know that 
it's a good idea to go looking up PCDs in an exception handler.)  Or 
maybe there's a more straightforward way  It would have to take AP 
stacks into account, and probably SetJump/LongJump as well.  That may or 
may not be simpler than the current code


More generally, I'd like to see some sort of platform-specific callout 
to further validate addresses.  Not all mapped addresses, or addresses 
up to the architectural limit, are safe to access.  For instance, reads 
to SMRAM outside of SMM will cause exceptions.  Also, we wouldn't want 
to go backtracing through MMIO or MMCFG space:  reads there could 
potentially have side effects on the hardware.


The rules can also vary at different points in boot.  For example, 
before memory is initialized, Intel Xeon processors generally execute 
32-bit code in cache-as-RAM mode, where the caches are jury-rigged to 
operate as temporary storage while the memory setup code is running.  In 
CAR mode, only a few address ranges can be accessed without causing 
machine checks:  the cache-as-RAM range containing the stack, heap, and 
HOB list, the architectural firmware range below 4G, and a few specific 
MMCFG and MMIO ranges.


So I'd like to suggest that you define an AddressValidationLib library 
class, which provides a routine which takes an address (or an address 
range?) and an indication of the intended use (memory read, memory 
write, execute/disassemble code, stack dump, IO, ...), and returns a 
value specifying if the access is:

- safe (IsLinearAddressValid() should return TRUE)
- unsafe (IsLinearAddressValid() should return FALSE)
- unknown (IsLinearAddressValid() should perform its other tests)

You can supply a NULL instance which always returns "unknown" for 
platforms which don't want to perform their own validation.


Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise
brian.john...@hpe.com

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Paulo Alcantara]

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 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
 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 
+#include 
+
 //
 // 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 UINTNCr3,
+  IN UINT8MaxPhyAddrBits,
+  IN UINTNLinearAddress
+  )
+{
+  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 UINTNCr3,
+  IN UINTNCr4,
+  IN UINTNLinearAddress
+  )
+{
+  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
+