Pass page fault code to Mm(Arm)AccessFault and implement NX support #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ThFabba
reviewed
Jan 1, 2018
ntoskrnl/include/internal/amd64/mm.h
Outdated
| //#define MI_IS_NOT_PRESENT_FAULT(FaultCode) !BooleanFlagOn(FaultCode, 0x1) | ||
| //#define MI_IS_PROTECTION_FAULT(FaultCode) BooleanFlagOn(FaultCode, 0x1) | ||
| #define MI_IS_WRITE_ACCESS(FaultCode) BooleanFlagOn(FaultCode, 0x2) | ||
| //#define MI_IS_SUPERVISOR_ACCESS(FaultCode) BooleanFlagOn(FaultCode, 0x4) |
ThFabba
reviewed
Jan 1, 2018
ntoskrnl/include/internal/i386/mm.h
Outdated
| //#define MI_IS_NOT_PRESENT_FAULT(FaultCode) !BooleanFlagOn(FaultCode, 0x1) | ||
| //#define MI_IS_PROTECTION_FAULT(FaultCode) BooleanFlagOn(FaultCode, 0x1) | ||
| #define MI_IS_WRITE_ACCESS(FaultCode) BooleanFlagOn(FaultCode, 0x2) | ||
| //#define MI_IS_SUPERVISOR_ACCESS(FaultCode) BooleanFlagOn(FaultCode, 0x4) |
ThFabba
reviewed
Jan 1, 2018
ntoskrnl/mm/ARM3/pagfault.c
Outdated
| @@ -1776,6 +1777,17 @@ MmArmAccessFault(IN BOOLEAN StoreInstruction, | |||
| 11); | |||
| } | |||
| } | |||
|
|
|||
| /* Check for execution of non-executable memory */ | |||
| if ((MI_IS_INSTRUCTION_FETCH(FaultCode)) && | |||
There was a problem hiding this comment.
Any reason for double parentheses?
ThFabba
reviewed
Jan 1, 2018
| @@ -1339,7 +1339,7 @@ KiTrap0EHandler(IN PKTRAP_FRAME TrapFrame) | |||
| NotSListFault: | |||
|
|
|||
| /* Call the access fault handler */ | |||
| Status = MmAccessFault(Present, | |||
| Status = MmAccessFault(TrapFrame->ErrCode, | |||
ThFabba
reviewed
Jan 1, 2018
ntoskrnl/mm/mmfault.c
Outdated
| } | ||
|
|
||
| /* Keep same old ReactOS Behaviour */ | ||
| if (StoreInstruction) | ||
| if (MI_IS_WRITE_ACCESS(FaultCode)) |
There was a problem hiding this comment.
This should be !MI_IS_NOT_PRESENT_FAULT unless you're planning on changing the semantics
ThFabba
reviewed
Jan 1, 2018
ntoskrnl/mm/ARM3/pagfault.c
Outdated
| IN PVOID Address, | ||
| IN KPROCESSOR_MODE Mode, | ||
| IN PVOID TrapInformation) | ||
| { | ||
| KIRQL OldIrql = KeGetCurrentIrql(), LockIrql; | ||
| BOOLEAN StoreInstruction = MI_IS_WRITE_ACCESS(FaultCode); |
There was a problem hiding this comment.
This is also a semantic change, albeit one that makes the code behave as it should. But perhaps start with "present fault" here and make it a "write access" check in its own commit for easier regression testing?
tkreuzer
force-pushed
the
amd64/pagefault_NX
branch
from
bee29ef
to
9ccaae9
Compare
Note: before we had a BOOLEAN parameter called StoreInstruction, but in reality it was not specifying whether the fault was from a store store instruction, but whether it was an access violation rather than a page-not-present fault. On x86 without PAE there are only 2 kinds of access violations: (1) Access of a kernel mode page from user mode, which is handled early and (2) access of a read-only (or COW) page with a writing instruction. Therefore we could get away with this, even though it relied on the wrong assumption that a fault, which was not a page-not-present-fault, was automatically a write access. This commit only changes one thing: we pass the full fault-code to MmAccessFault and handle the rest from there in exactly the same way as before. More changes are coming to make things clear.
…te it there to what was declared as "StoreInstruction" No functional changes.
…_WRITE_ACCESS macros in MmArmAccessFault.
tkreuzer
force-pushed
the
amd64/pagefault_NX
branch
from
9ccaae9
to
d97e0c2
Compare
tkreuzer
force-pushed
the
amd64/pagefault_NX
branch
from
d97e0c2
to
deb2d1c
Compare
ThFabba
approved these changes
Jan 5, 2018
ThFabba
reviewed
Jan 5, 2018
| @@ -1325,7 +1333,7 @@ MiDispatchFault(IN BOOLEAN StoreInstruction, | |||
| } | |||
|
|
|||
| /* Resolve the fault -- this will release the PFN lock */ | |||
| Status = MiResolveProtoPteFault(StoreInstruction, | |||
| Status = MiResolveProtoPteFault(!MI_IS_NOT_PRESENT_FAULT(FaultCode), | |||
There was a problem hiding this comment.
Do you want to mark these with a FIXME or something (since presumably they're meant to check for write instead)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the parameter passing to Mm(Arm)AccessFault and also implements support for NX faults where the architecture supports it (x64 and x86+PAE).
Note: before we had a BOOLEAN parameter called StoreInstruction, but in reality it was not specifying whether the fault was from a store store instruction, but whether it was an access violation rather than a page-not-present fault. On x86 without PAE there are only 2 kinds of access violations: (1) Access of a kernel mode page from user mode, which is handled early and (2) access of a read-only (or COW) page with a writing instruction. Therefore we could get away with this, even though it relied on the wrong assumption that a fault, which was not a page-not-present-fault, was automatically a write access.