[PATCH v2] ntdll: Fix overflow if running 32bit app w/ wine64
wine64 was using IMAGE_NT_HEADERS to access header information regardless of the execution type; hence it would use IMAGE_OPTIONAL_HEADER64 for a 32bit app. This could result in an overflow and a request to mmap for a huge amount of memory causing an out of memory error. This patch ensures IMAGE_OPTIONAL_HEADER32 is used for a 32-bit app and IMAGE_OPTIONAL_HEADER64 is used for a 64-bit app Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> --- Fixed the formatting - sorry about that dlls/ntdll/virtual.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index af1509eae5d..a05000f2e2b 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -1933,9 +1933,26 @@ NTSTATUS virtual_alloc_thread_stack( TEB *teb, SIZE_T reserve_size, SIZE_T commi if (!reserve_size || !commit_size) { - IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress ); - if (!reserve_size) reserve_size = nt->OptionalHeader.SizeOfStackReserve; - if (!commit_size) commit_size = nt->OptionalHeader.SizeOfStackCommit; + struct nt + { + DWORD Signature; + IMAGE_FILE_HEADER FileHeader; + union + { + IMAGE_OPTIONAL_HEADER32 hdr32; + IMAGE_OPTIONAL_HEADER64 hdr64; + } opt; + }; + + struct nt *nt = (struct nt*) RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress ); + if (nt->opt.hdr32.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + if (!reserve_size) reserve_size = nt->opt.hdr32.SizeOfStackReserve; + if (!commit_size) commit_size = nt->opt.hdr32.SizeOfStackCommit; + } else { + if (!reserve_size) reserve_size = nt->opt.hdr64.SizeOfStackReserve; + if (!commit_size) commit_size = nt->opt.hdr64.SizeOfStackCommit; + } + TRACE("reserve_size: %lu, commit_size: %lu", reserve_size, commit_size); } size = max( reserve_size, commit_size ); -- 2.17.1
I'm following up on this to patch to better illustrate the problem. If you run the bash script I have included below - it will create two applications: 1. mono32.exe: PE32 executable (console) Intel 80386 Mono/.Net assembly, for MS Windows; and 2. c64.exe: PE32+ executable (console) x86-64, for MS Windows mono32 being a 32bit mono application and c64 being a 64bit 'C' application (this emulates the set-up of the launcher and application I'm trying to get working). When ran, mono32 grabs the MachineGuid from the registry. It then passes it to c64.exe, which grabs the same registry value and compares them. These executables work under Windows but do not work under Wine. This is because if you run: wine mono32 mono32 will get a different value to c64. This is because 'wine mono32' will fetch the value from the WoW6432 branch. This currently has a different value to the 64bit branch. (I have already submitted a patch to ensure these values are the same - but it was rightly rejected. As Nikolay Sivov pointed out - Windows doesn't have a WoW6432 entry and neither should Wine. Another patch has been submitted for that). But if you run: wine64 mono32 you get the Out of Memory error that is fixed by this patch. Applying this patch allows these applications to work as they currently do under Windows. So apart from manually copying the 64bit MachineGuid entry to the WoW6432 branch, then without this patch, I'm not aware of any other solution. Please let me know if there is one. Below is the bash script that demonstrates the issue (you can just copy and paste the whole lot): # create mono source cat << 'END' > mono32.cs using Microsoft.Win32; using System.Diagnostics; using System.Reflection; using System.IO; using System; public class Launcher { static public void Main () { RegistryKey rk = Registry.LocalMachine.OpenSubKey("SOFTWARE\\Microsoft\\Cryptography"); if (rk != null) { object guid = rk.GetValue("MachineGuid"); if (guid != null) { string strGuid = guid.ToString(); string cwd = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); ProcessStartInfo processStartInfo = new ProcessStartInfo(); processStartInfo.FileName = cwd + "\\c64.exe"; processStartInfo.Arguments = strGuid; processStartInfo.UseShellExecute = false; Process.Start(processStartInfo); } else { Console.WriteLine ("guid not found - run under 'wine64'"); } } else { Console.WriteLine("Mono couldn't find 'HKLM\\SOFTWARE\\Microsoft\\Cryptography'"); } } } END # create 'C' source cat << 'END' > c64.c #include <windows.h> #include <stdio.h> int main(int argc, const char *argv[]) { LONG rc; BYTE data[200]; DWORD data_size = 200; HKEY hkey; if (argc != 2) printf("Usage: c64.exe guid\n"); if (RegOpenKeyExA(HKEY_LOCAL_MACHINE, "Software\\Microsoft\\Cryptography", 0, KEY_QUERY_VALUE, &hkey) == ERROR_SUCCESS) { rc = RegQueryValueExA(hkey, "MachineGuid", NULL, NULL, data, &data_size); if (rc == ERROR_SUCCESS) { if (strcmp(data, argv[1]) == 0) { printf("We match - I work\n"); } else { printf("We don't match. I got:\n%s\nMono gave me:\n%s\n", data, argv[1]); } } else { printf("guid not found\n", rc); } } else { printf("c64 couldn't find 'HKLM\\SOFTWARE\\Microsoft\\Cryptography'\n"); } } END # compile mono mcs mono32.cs # compile 'C' x86_64-w64-mingw32-gcc c64.c -o c64.exe # try wine (always fails as the guid is wrong or missing) wine mono32 # try wine64 (fails with out of memory unless the overflow patch is applied) wine64 mono32 On 17/10/18 8:49 pm, Brendan McGrath wrote:
wine64 was using IMAGE_NT_HEADERS to access header information regardless of the execution type; hence it would use IMAGE_OPTIONAL_HEADER64 for a 32bit app.
This could result in an overflow and a request to mmap for a huge amount of memory causing an out of memory error.
This patch ensures IMAGE_OPTIONAL_HEADER32 is used for a 32-bit app and IMAGE_OPTIONAL_HEADER64 is used for a 64-bit app
Signed-off-by: Brendan McGrath <brendan(a)redmandi.com> ---
Fixed the formatting - sorry about that
dlls/ntdll/virtual.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index af1509eae5d..a05000f2e2b 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -1933,9 +1933,26 @@ NTSTATUS virtual_alloc_thread_stack( TEB *teb, SIZE_T reserve_size, SIZE_T commi
if (!reserve_size || !commit_size) { - IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress ); - if (!reserve_size) reserve_size = nt->OptionalHeader.SizeOfStackReserve; - if (!commit_size) commit_size = nt->OptionalHeader.SizeOfStackCommit; + struct nt + { + DWORD Signature; + IMAGE_FILE_HEADER FileHeader; + union + { + IMAGE_OPTIONAL_HEADER32 hdr32; + IMAGE_OPTIONAL_HEADER64 hdr64; + } opt; + }; + + struct nt *nt = (struct nt*) RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress ); + if (nt->opt.hdr32.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + if (!reserve_size) reserve_size = nt->opt.hdr32.SizeOfStackReserve; + if (!commit_size) commit_size = nt->opt.hdr32.SizeOfStackCommit; + } else { + if (!reserve_size) reserve_size = nt->opt.hdr64.SizeOfStackReserve; + if (!commit_size) commit_size = nt->opt.hdr64.SizeOfStackCommit; + } + TRACE("reserve_size: %lu, commit_size: %lu", reserve_size, commit_size); }
size = max( reserve_size, commit_size );
Brendan McGrath <brendan(a)redmandi.com> writes:
But if you run: wine64 mono32
you get the Out of Memory error that is fixed by this patch. Applying this patch allows these applications to work as they currently do under Windows.
For .NET binaries this should be solved by generating a PE64 header to replace the PE32 one. -- Alexandre Julliard julliard(a)winehq.org
you get the Out of Memory error that is fixed by this patch. Applying this patch allows these applications to work as they currently do under Windows.
For .NET binaries this should be solved by generating a PE64 header to replace the PE32 one.
Does the test case work with native .NET? If not, we need to fix that first. If it does, we should change the header in _CorValidateImage.
I installed .NET 4.0 and it doesn't work. It's the same error. It works however with this patch. On 19/10/18 6:10 am, Vincent Povirk wrote:
you get the Out of Memory error that is fixed by this patch. Applying this patch allows these applications to work as they currently do under Windows. For .NET binaries this should be solved by generating a PE64 header to replace the PE32 one. Does the test case work with native .NET? If not, we need to fix that first.
If it does, we should change the header in _CorValidateImage.
I installed .NET 4.0 and it doesn't work. It's the same error. It works however with this patch.
If native mscoree is being used (you might want to double check) it suggests that either ntdll is calling _CorValidateImage too late, or this is the correct approach. I don't know how to test this.
I ran this test with WINEDEBUG=+relay with and without the patch. With the patch I see this entry: 0067:Call PE DLL (proc=0x7fe6ef3a8610,module=0x7fe6ef380000 L"mscoree.dll",reason=PROCESS_ATTACH,res=0x22fb00) Without the patch I don't (in fact the text 'mscoree' isn't in the log at all). So I think the crash happens before the mscoree.dll library is loaded. And I guess this makes sense since the issue is with allocation of memory for the stack of the main process. On 20/10/18 12:24 am, Vincent Povirk wrote:
I installed .NET 4.0 and it doesn't work. It's the same error. It works however with this patch. If native mscoree is being used (you might want to double check) it suggests that either ntdll is calling _CorValidateImage too late, or this is the correct approach. I don't know how to test this.
Did you mean something like the below? This also fixes my test case - but I feel like my original patch was a lot safer (obviously the 'imports' reference should be fixed regardless). diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index af1509eae5d..9be494ba57f 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -1364,7 +1364,7 @@ static NTSTATUS map_image( HANDLE hmapping, ACCESS_MASK access, int fd, SIZE_T m IMAGE_NT_HEADERS *nt; IMAGE_SECTION_HEADER sections[96]; IMAGE_SECTION_HEADER *sec; - IMAGE_DATA_DIRECTORY *imports; + IMAGE_DATA_DIRECTORY *imports = NULL; NTSTATUS status = STATUS_CONFLICTING_ADDRESSES; SIZE_T header_size, total_size = image_info->map_size; int i; @@ -1423,7 +1423,47 @@ static NTSTATUS map_image( HANDLE hmapping, ACCESS_MASK access, int fd, SIZE_T m memcpy(sections, header_start, sizeof(*sections) * nt->FileHeader.NumberOfSections); sec = sections; - imports = nt->OptionalHeader.DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT; + if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC && is_win64 && + !(nt->FileHeader.Characteristics & IMAGE_FILE_DLL)) { + IMAGE_OPTIONAL_HEADER32 *hdr32 = (IMAGE_OPTIONAL_HEADER32 *) &nt->OptionalHeader; + /* check if there is room in the header for generating a PE64 header, + * if not - we'll leave it and hope for the best */ + if (header_start + sizeof(IMAGE_OPTIONAL_HEADER64) - sizeof(IMAGE_OPTIONAL_HEADER32) + + sizeof(*sections) * nt->FileHeader.NumberOfSections <= header_end) { + /* We'll convert the IMAGE_OPTIONAL_HEADER32 to IMAGE_OPTIONAL_HEADER64 + * we'll splat the sections area - but this was just copied. + * we copy from back to front so we don't splat the data we still need */ + memmove(nt->OptionalHeader.DataDirectory, hdr32->DataDirectory, + hdr32->NumberOfRvaAndSizes * sizeof(hdr32->DataDirectory[0])); + nt->OptionalHeader.NumberOfRvaAndSizes = hdr32->NumberOfRvaAndSizes; + nt->OptionalHeader.LoaderFlags = hdr32->LoaderFlags; + nt->OptionalHeader.SizeOfHeapCommit = hdr32->SizeOfHeapCommit; + nt->OptionalHeader.SizeOfHeapReserve = hdr32->SizeOfHeapReserve; + nt->OptionalHeader.SizeOfStackCommit = hdr32->SizeOfStackCommit; + nt->OptionalHeader.SizeOfStackReserve = hdr32->SizeOfStackReserve; + nt->OptionalHeader.ImageBase = hdr32->ImageBase; + /* the rest of the values align between the 32bit and 64bit optional header */ + + /* update the magic value and the size of optional header */ + nt->OptionalHeader.Magic = IMAGE_NT_OPTIONAL_HDR64_MAGIC; + nt->FileHeader.SizeOfOptionalHeader += sizeof(IMAGE_OPTIONAL_HEADER64) + - sizeof(IMAGE_OPTIONAL_HEADER32); + + /* copy the sections back + * Will this will break apps that map over the top of the section headers? + * (hopefully none of these are 32bit .NET/Mono apps anyway) */ + memcpy((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader, + sections, sizeof(*sections) * nt->FileHeader.NumberOfSections); + } else { + /* even though we can't create a PE64 header, we can still make sure + * imports points to the correct location */ + imports = hdr32->DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT; + } + } + + if (!imports) + imports = nt->OptionalHeader.DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT; + if (!imports->Size || !imports->VirtualAddress) imports = NULL; /* check for non page-aligned binary */ On 19/10/18 5:32 am, Alexandre Julliard wrote:
Brendan McGrath <brendan(a)redmandi.com> writes:
But if you run: wine64 mono32
you get the Out of Memory error that is fixed by this patch. Applying this patch allows these applications to work as they currently do under Windows. For .NET binaries this should be solved by generating a PE64 header to replace the PE32 one.
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=43495 Your paranoid android. === debian9 (build log) === error: corrupt patch at line 10 Task: Patch failed to apply === debian9 (build log) === error: corrupt patch at line 10 Task: Patch failed to apply
Brendan McGrath <brendan(a)redmandi.com> writes:
Did you mean something like the below? This also fixes my test case - but I feel like my original patch was a lot safer (obviously the 'imports' reference should be fixed regardless).
I've committed a different fix, but yes, that's the general idea. -- Alexandre Julliard julliard(a)winehq.org
Awesome. I just tired it out and it works well. Thanks very much. On 26/10/18 7:27 am, Alexandre Julliard wrote:
Brendan McGrath <brendan(a)redmandi.com> writes:
Did you mean something like the below? This also fixes my test case - but I feel like my original patch was a lot safer (obviously the 'imports' reference should be fixed regardless). I've committed a different fix, but yes, that's the general idea.
participants (4)
-
Alexandre Julliard -
Brendan McGrath -
Marvin -
Vincent Povirk