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

[Paulo Alcantara <paulo@paulo.ac>]

"Brian J. Johnson" <brian.johnson@hpe.com> 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 > 0xFFFFFFFFFULL) ||
>> +        LinearAddress > 0xFFFFFFFF) {
>> +      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 > 0xFFFFFFFFFULL ||
>         ((Cr4 & BIT5) == 0 && LinearAddress > 0xFFFFFFFF)) {
>       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