[PATCH] ntoskrnl: Have MmIsAddressValid use IsBadReadPtr.

[Derek Lesho]

Hi Thomas, in your example, does &Base refer to the base of
ntoskrnl.exe?  If so, why are you allocating a buffer there, wouldn't
that corrupt the memory of ntoskrnl, or am I misunderstanding what
your code is doing.  On windows, do drivers have access to change the
memory of ntoskrnl?

Atleast in EAC, they find the address of a function inside of the
ntoskrnl address space, and continuously decrement the address until
MMisAddressValid returns FALSE.  Without my patch, wine always returns
FALSE to EAC, even on the first address check.

On Wed, Jun 6, 2018 at 3:51 AM, Thomas Faber <thomas.faber at reactos.org> wrote:
>
> On 2018-06-06 04:36, Zhiyi Zhang wrote:
> > The function is meant to be used in nt kernel space, however wineserver is
> > in user space as far as I know. So we would need different implementation.
> >
> > May be we could do something like return !IsBadWritePtr(VirtualAddress, 1) || !IsBadReadPtr(VirtualAddress, 1).
> > And write a test to verify such behavior.
>
> IsBadReadPtr(x) implies IsBadWritePtr(x), since Windows does not have a
> concept of "write-only" access to pages. So checking both seems
> unnecessary.
> The real difference between IsBadReadPtr and MmIsAddressValid is that
> the former can actually _make_ the address valid by causing a page
> fault (and paging in or zeroing the page). MmIsAddressValid on the
> other hand would simply look at the current state of the page table
> entry without side effects.
>
> This stuff isn't hard to test per se, but a full test that's correct for
> Windows kernel land will not currently work in Wine due to unimplemented
> Mm stuff, e.g.
>
> for (protection in [PAGE_NOACCESS, PAGE_READONLY, PAGE_READWRITE])
> {
> ZwAllocateVirtualMemory(..., &Base, ..., MEM_COMMIT, protection);
> // physical page is not assigned until first access
> todo_wine ok(!MmIsAddressValid(Base, 1), ...);
> // force a physical page to appear and prevent it from getting paged out
> mdl = IoAllocateMdl(Base, 1, FALSE, FALSE, NULL);
> __TRY { MmProbeAndLockPages(mdl, UserMode, IoModifyAccess); todo_wine ok(protection != PAGE_NOACCESS, ...); }
> __EXCEPT_PAGE_FAULT { ok(protection == PAGE_NOACCESS, ...); goto Next; }
> // the system address is now guaranteed to be valid
> todo_wine ok(MmIsAddressValid(MmGetSystemAddressForMdlSafe(mdl, NormalPagePriority), 1), ...);
> MmUnlockPages...
> Next:
> IoFreeMdl...
> ZwFreeVirtualMemory...
> }
>
> ... has a todo with pretty much every ok(), with or without the patch :\
>
>
> With some luck, the simpler
> void test_function()
> {
> ok(MmIsAddressValid((void*)test_function, ...); // fails in master, succeeds with patch?
> ptr = ExAllocatePoolWithTag(NonPagedPool, ...)
> ok(MmIsAddressValid(ptr, ...);
> ExFreePoolWithTag...
> ok(!MmIsAddressValid(NULL), ...);
> }
>
> might be enough to show that the patch is correct though.