From: "Brian J. Johnson" <brian.johnson@hpe.com>
To: Paulo Alcantara <paulo@paulo.ac>, <edk2-devel@lists.01.org>
Cc: Laszlo Ersek <lersek@redhat.com>, Eric Dong <eric.dong@intel.com>
Subject: Re: [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses
Date: Wed, 3 Jan 2018 10:59:24 -0600 [thread overview]
Message-ID: <27dd187f-3b1b-a325-6fd0-84d78f1ff28f@hpe.com> (raw)
In-Reply-To: <32f06077006939f71560970f6abcbbb2062ea5c3.1514517573.git.paulo@paulo.ac>
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;
}
(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.johnson@hpe.com
next prev parent reply other threads:[~2018-01-03 16:54 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-14 12:47 [RFC 0/1] Stack trace support in X64 exception handling Paulo Alcantara
2017-11-14 12:47 ` [RFC 1/1] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Paulo Alcantara
2017-11-14 14:01 ` Andrew Fish
2017-11-14 14:26 ` 答复: " Fan Jeff
2017-11-14 14:38 ` Andrew Fish
2017-11-14 15:30 ` Paulo Alcantara
2017-11-14 16:51 ` Brian J. Johnson
2017-12-29 3:48 ` [RFC v4 0/6] Stack trace support in X64 exception handling Paulo Alcantara
2017-12-29 4:39 ` [RFC v4 1/6] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Paulo Alcantara
2018-01-03 8:53 ` 答复: " Fan Jeff
2018-01-03 14:51 ` Paulo Alcantara
2017-12-29 4:39 ` [RFC v4 2/6] UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() Paulo Alcantara
2017-12-29 4:39 ` [RFC v4 3/6] UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support Paulo Alcantara
2017-12-29 4:39 ` [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses Paulo Alcantara
2018-01-03 8:42 ` 答复: " Fan Jeff
2018-01-03 14:45 ` Paulo Alcantara
2018-01-03 16:59 ` Brian J. Johnson [this message]
2018-01-04 13:03 ` Paulo Alcantara
2018-01-04 1:36 ` Yao, Jiewen
2018-01-04 1:58 ` Yao, Jiewen
2018-01-04 13:29 ` Paulo Alcantara
2018-01-04 14:35 ` Yao, Jiewen
2018-01-04 15:15 ` Paulo Alcantara
2018-01-04 13:18 ` Paulo Alcantara
2017-12-29 4:39 ` [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers Paulo Alcantara
2018-01-03 8:45 ` 答复: " Fan Jeff
2018-01-03 14:48 ` Paulo Alcantara
2018-01-04 1:07 ` Yao, Jiewen
2017-12-29 4:39 ` [RFC v4 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses Paulo Alcantara
2018-01-03 8:46 ` 答复: " Fan Jeff
2018-01-04 0:59 ` [RFC v4 0/6] Stack trace support in X64 exception handling Yao, Jiewen
2018-01-04 13:36 ` Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 0/8] " Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 1/8] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 2/8] UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 3/8] UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 4/8] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to validate memory addresses Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 5/8] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 6/8] UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 7/8] UefiCpuPkg/CpuExceptionHandlerLib: Validate memory address ranges Paulo Alcantara
2018-01-15 0:23 ` [RFC v5 8/8] UefiCpuPkg/CpuExceptionHandlerLib: Add early check in DumpStackContents Paulo Alcantara
2018-01-17 12:57 ` [RFC v5 0/8] Stack trace support in X64 exception handling Yao, Jiewen
2018-01-17 22:48 ` Yao, Jiewen
2018-01-19 0:09 ` Paulo Alcantara
2018-01-19 0:02 ` Paulo Alcantara
2018-01-19 0:15 ` Paulo Alcantara
2018-01-29 13:38 ` Paulo Alcantara
2018-01-31 5:56 ` Yao, Jiewen
2018-01-31 19:05 ` Paulo Alcantara
2017-11-14 13:21 ` [RFC 0/1] " Paulo Alcantara
2017-11-14 14:03 ` 答复: " Fan Jeff
2017-11-14 14:12 ` 答复: " Fan Jeff
2017-11-14 15:37 ` Paulo Alcantara
2017-11-14 16:33 ` Brian J. Johnson
2017-11-14 17:23 ` Andrew Fish
2017-11-14 17:41 ` Brian J. Johnson
2017-11-14 17:56 ` Paulo Alcantara
2017-11-15 13:21 ` 答复: 答复: " Fan Jeff
2017-11-15 14:41 ` Paulo Alcantara
2017-11-15 14:52 ` 答复: " Fan Jeff
2017-11-16 1:18 ` [RFC v2 0/3] " Paulo Alcantara
2017-11-16 1:18 ` [RFC v2 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Paulo Alcantara
2017-11-16 1:57 ` Yao, Jiewen
2017-11-16 22:13 ` Paulo Alcantara
2017-11-17 3:43 ` Yao, Jiewen
2017-11-20 14:51 ` Paulo Alcantara
2017-11-16 15:43 ` Brian J. Johnson
2017-11-16 22:19 ` Paulo Alcantara
2017-11-16 1:18 ` [RFC v2 2/3] UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() Paulo Alcantara
2017-11-16 1:18 ` [RFC v2 3/3] UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support Paulo Alcantara
2017-11-16 1:46 ` [RFC v2 0/3] Stack trace support in X64 exception handling Paulo Alcantara
2017-11-16 5:01 ` Andrew Fish
2017-11-16 22:02 ` Paulo Alcantara
2017-11-16 21:56 ` [RFC v3 " Paulo Alcantara
2017-11-16 21:56 ` [RFC v3 1/3] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support Paulo Alcantara
2017-11-17 7:24 ` 答复: " Fan Jeff
2017-11-20 14:59 ` Paulo Alcantara
2017-11-23 14:27 ` 答复: " Fan Jeff
2017-11-23 18:34 ` Andrew Fish
2017-11-23 19:49 ` Fan Jeff
2017-11-16 21:56 ` [RFC v3 2/3] UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName() Paulo Alcantara
2017-11-16 21:56 ` [RFC v3 3/3] UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support Paulo Alcantara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27dd187f-3b1b-a325-6fd0-84d78f1ff28f@hpe.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox