[PATCH] ntoskrnl.exe: fix handling relocations on page boundary

[Rafał H]

Code is only used for non-page-aligned device drivers.
Previously if relocation was happening on page boundary it crashed
because only one page had changed protection in load_driver_module.
Now code is changing protection for two pages (current and next).
Also added "goto error" for VirtualProtect call for the current page. This
change is safe because if it failed before this change updating the page
content would crash anyway.
Commit fixes running MTA: San Andreas game.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46401
Signed-off-by: Rafał Harabień <rafalh92(a)outlook.com>
---
 dlls/ntoskrnl.exe/ntoskrnl.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
index 48b309f3d6..0c32e12801 100644
--- a/dlls/ntoskrnl.exe/ntoskrnl.c
+++ b/dlls/ntoskrnl.exe/ntoskrnl.c
@@ -3318,8 +3318,9 @@ static HMODULE load_driver_module( const WCHAR *name )
     if (nt->OptionalHeader.SectionAlignment < info.PageSize ||
         !(nt->FileHeader.Characteristics & IMAGE_FILE_DLL))
     {
-        DWORD old;
+        DWORD old, old_next_page;
         IMAGE_BASE_RELOCATION *rel, *end;
+        void *page, *next_page = NULL;
 
         if ((rel = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_BASERELOC, &size )))
         {
@@ -3327,13 +3328,33 @@ static HMODULE load_driver_module( const WCHAR *name )
             end = (IMAGE_BASE_RELOCATION *)((char *)rel + size);
             while (rel < end && rel->SizeOfBlock)
             {
-                void *page = (char *)module + rel->VirtualAddress;
-                VirtualProtect( page, info.PageSize, PAGE_EXECUTE_READWRITE, &old );
+                page = (char *)module + rel->VirtualAddress;
+                /* if the page protection was already changed in previous interation don't do it again here */
+                if (page == next_page)
+                    old = old_next_page;
+                else
+                {
+                    /* bring back old protection for 'next_page' from previous iteration */
+                    if (old_next_page != PAGE_EXECUTE_READWRITE)
+                        VirtualProtect( next_page, info.PageSize, old_next_page, &old_next_page );
+                    /* change current page protection */
+                    if (!VirtualProtect( page, info.PageSize, PAGE_EXECUTE_READWRITE, &old ))
+                        goto error;
+                }
+
+                /* protect the next page as well because relocation can occur on the page boundary */
+                next_page = (char*)page + info.PageSize;
+                if (!VirtualProtect( next_page, info.PageSize, PAGE_EXECUTE_READWRITE, &old_next_page ))
+                    next_page = NULL;
+
                 rel = LdrProcessRelocationBlock( page, (rel->SizeOfBlock - sizeof(*rel)) / sizeof(USHORT),
                                                  (USHORT *)(rel + 1), delta );
+                /* bring back old protection of current page; next page is handled in the next iteration */
                 if (old != PAGE_EXECUTE_READWRITE) VirtualProtect( page, info.PageSize, old, &old );
                 if (!rel) goto error;
             }
+            if (next_page && old_next_page != PAGE_EXECUTE_READWRITE)
+                VirtualProtect( next_page, info.PageSize, old_next_page, &old_next_page );
             /* make sure we don't try again */
             size = FIELD_OFFSET( IMAGE_NT_HEADERS, OptionalHeader ) + nt->FileHeader.SizeOfOptionalHeader;
             VirtualProtect( nt, size, PAGE_READWRITE, &old );
-- 
2.17.1

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=46034

Your paranoid android.


=== debian9 (32 bit report) ===

ntoskrnl.exe:
ntoskrnl: Timeout

[Alexandre Julliard]

Rafał H <rafalh92(a)outlook.com> writes:
> Code is only used for non-page-aligned device drivers.
> Previously if relocation was happening on page boundary it crashed
> because only one page had changed protection in load_driver_module.
> Now code is changing protection for two pages (current and next).
> Also added "goto error" for VirtualProtect call for the current page. This
> change is safe because if it failed before this change updating the page
> content would crash anyway.

You should be able to simply duplicate the perform_relocations()
functions from the standard loader.

-- 
Alexandre Julliard
julliard(a)winehq.org