12 Oct
2014
12 Oct
'14
4:16 a.m.
The magic of 2 is to account for the Wow64 PEB, on 64-bit systems, FYI.
I agree on your other points -- the 'test' was not "prove me this generates
the same random numbers as windows" but rather "make sure this actually
generates enough randomness"
Best regards,
Alex Ionescu
On Sat, Oct 11, 2014 at 4:59 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:
The improvement is based on readability / coding style. I think the
comment explained the reason to improve it. But I can explain again in more
detail:
The old code created a random value for a page offset inside a 64k region.
The PEB was supposed to get into that region, but preferably not below.
Obviously, we don't have ALL of these 16 possible 4k page locations
available, if we neither want to go above the upper or below the lower
margin, but still fit n pages between these 2.
The old code "fixed" the random value to account for that limitation by
hardcoding a 2 (for 2 pages), making sure that there are 2 pages available
to put the PEB in.
The new code will check the size of the PEB instead of relying on the
hardcoded magic value of 2, wich wasn't even explained anywhere. So all I
did here was remove a hack.
The rest of the change affects the path that we follow on a failure.
Instead of trying to allocate the PEB at a constant given address and if
that fails, try again from top down, we use the upper margin and try to
allocate at the highest address from there. So it cannot fail, unless the
whole address space is blocked. The commit message might not have been
describing this properly, but the claim that there is no change in Windows
behaviour still stays, since there is no way to predict the address of the
PEB anyway. It's random, from top down. And it stays that way. Under normal
circumstances only the NLS section would block the address range and in
that case the allocation will go below, so no change at all. When it comes
to cloned processes, things might be more complicated, but then there is no
chance to predict the location of the PEB anyway, it could go anywhere,
even below the 64k range. Again no change.
If you still have doubts, please let me know what exactly you think could
be wrong here, so I can address that accordingly.
Timo
PS: we are talking about randomized behavior here, that is done for
security reasons, so doing it differently without breaking assumptions that
user mode applications can make would most likely be beneficial. When you
write a security software that has prevention capabilities, you also also
change Windows behaviour. If that had a negative effect on *legitimate*
software running on the system, it would be bad. If it doesn't have any
negative effect, or only on "bad" software, it's good. If you can
reasonably argue, why this change could possibly affect legitimate software
in a negative way, then I can write a test based on that.
Am 11.10.2014 18:37, schrieb Alex Ionescu:
Where't the unit test proving your 'improved' algorithm matches XP
SP2/SRV03 SP1?
Best regards,
Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, <tkreuzer(a)svn.reactos.org> wrote:
Author: tkreuzer
Date: Wed Oct 8 00:31:35 2014
New Revision: 64591
URL: http://svn.reactos.org/svn/reactos?rev=64591&view=rev
Log:
[NTOSKRNL]
- Improve the random address base code in MiCreatePebOrTeb to actually
make sense and not rely on retarded hacks implicitly hardcoding the PEB
size in pages into the random value generation.
Modified:
trunk/reactos/ntoskrnl/mm/ARM3/procsup.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/procsup.c…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] Wed Oct 8
00:31:35 2014
@@ -48,13 +48,13 @@
NTAPI
MiCreatePebOrTeb(IN PEPROCESS Process,
IN ULONG Size,
- OUT PULONG_PTR Base)
+ OUT PULONG_PTR BaseAddress)
{
PETHREAD Thread = PsGetCurrentThread();
PMMVAD_LONG Vad;
NTSTATUS Status;
- ULONG RandomCoeff;
- ULONG_PTR StartAddress, EndAddress;
+ ULONG_PTR HighestAddress, RandomBase;
+ ULONG AlignedSize;
LARGE_INTEGER CurrentTime;
TABLE_SEARCH_RESULT Result = TableFoundNode;
PMMADDRESS_NODE Parent;
@@ -83,25 +83,30 @@
/* Check if this is a PEB creation */
if (Size == sizeof(PEB))
{
- /* Start at the highest valid address */
- StartAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;
-
- /* Select the random coefficient */
+ /* Create a random value to select one page in a 64k region */
KeQueryTickCount(&CurrentTime);
- CurrentTime.LowPart &= ((64 * _1KB) >> PAGE_SHIFT) - 1;
- if (CurrentTime.LowPart <= 1) CurrentTime.LowPart = 2;
- RandomCoeff = CurrentTime.LowPart << PAGE_SHIFT;
-
- /* Select the highest valid address minus the random coefficient
*/
- StartAddress -= RandomCoeff;
- EndAddress = StartAddress + ROUND_TO_PAGES(Size) - 1;
+ CurrentTime.LowPart &= (_64K / PAGE_SIZE) - 1;
+
+ /* Calculate a random base address */
+ RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;
+ RandomBase -= CurrentTime.LowPart << PAGE_SHIFT;
+
+ /* Make sure the base address is not too high */
+ AlignedSize = ROUND_TO_PAGES(Size);
+ if ((RandomBase + AlignedSize) >
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1)
+ {
+ RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1 -
AlignedSize;
+ }
+
+ /* Calculate the highest allowed address */
+ HighestAddress = RandomBase + AlignedSize - 1;
/* Try to find something below the random upper margin */
Result = MiFindEmptyAddressRangeDownTree(ROUND_TO_PAGES(Size),
- EndAddress,
+ HighestAddress,
PAGE_SIZE,
&Process->VadRoot,
- Base,
+ BaseAddress,
&Parent);
}
@@ -113,7 +118,7 @@
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS,
PAGE_SIZE,
&Process->VadRoot,
- Base,
+ BaseAddress,
&Parent);
/* Bail out, if still nothing free was found */
if (Result == TableFoundNode)
@@ -125,12 +130,12 @@
}
/* Validate that it came from the VAD ranges */
- ASSERT(*Base >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
+ ASSERT(*BaseAddress >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
/* Build the rest of the VAD now */
- Vad->StartingVpn = (*Base) >> PAGE_SHIFT;
- Vad->EndingVpn = ((*Base) + Size - 1) >> PAGE_SHIFT;
- Vad->u3.Secured.StartVpn = *Base;
+ Vad->StartingVpn = (*BaseAddress) >> PAGE_SHIFT;
+ Vad->EndingVpn = ((*BaseAddress) + Size - 1) >> PAGE_SHIFT;
+ Vad->u3.Secured.StartVpn = *BaseAddress;
Vad->u3.Secured.EndVpn = (Vad->EndingVpn << PAGE_SHIFT) | (PAGE_SIZE
- 1);
Vad->u1.Parent = NULL;
@@ -146,7 +151,7 @@
Vad->ControlArea = NULL; // For Memory-Area hack
Vad->FirstPrototypePte = NULL;
DPRINT("VAD: %p\n", Vad);
- DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *Base,
Process->ImageFileName);
+ DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *BaseAddress,
Process->ImageFileName);
MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
/* Release the working set */
_______________________________________________
Ros-dev mailing listRos-dev@reactos.orghttp://www.reactos.org/mailman/listinfo/ros-dev
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev