[PATCH] ntdll: Fix overflow if running 32bit app w/ wine64

[Brendan McGrath]

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>
---

This fix is pretty much a copy and paste of code from server/mapping.c. But I'm
not sure if it is the right way to go. There may already be a sanitized
location for these values available within virtual_alloc_thread_stack
that I just couldn't find. 

 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..8acea14af93 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

[Dmitry Timoshkov]

Brendan McGrath <brendan(a)redmandi.com> wrote:
> +        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);

Indentation seems to be broken by shifting lower lines by 1 space left.

-- 
Dmitry.

[Dmitry Timoshkov]

Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
> Brendan McGrath <brendan(a)redmandi.com> wrote:
> > +        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);
>
> Indentation seems to be broken by shifting lower lines by 1 space left.

Oops, that's the tabs vs spaces thing. Please use spaces instead of tabs
to indent the new code.

-- 
Dmitry.

[Alexandre Julliard]

Brendan McGrath <brendan(a)redmandi.com> writes:
> 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.

wine64 should not be used for a 32-bit binary, it should exec the 32-bit
loader instead. Do you have a sample app to reproduce this?

-- 
Alexandre Julliard
julliard(a)winehq.org

[Brendan McGrath]

It seems to be a problem with any 32-bit Mono app ran with 'wine64'. I 
think it converts the bytecode to 64-bit machine code (even though the 
header is PE32).

You can recreate by copy and pasting the following:
# create csharp code
cat << END > helloworld.cs
using Microsoft.Win32;
using System;

public class HelloWorld
{
     static public void Main ()
     {
         Console.WriteLine ("Hello World");
     }
}
END

# compile
mcs helloworld.cs
mcs -platform:x64 helloworld.cs -out:helloworld64.exe

# confirm headers
file helloworld.exe helloworld64.exe

# Output should be
# helloworld.exe:   PE32 executable (console) Intel 80386 Mono/.Net 
assembly, for MS Windows
# helloworld64.exe: PE32+ executable (console) x86-64 Mono/.Net 
assembly, for MS Windows

# run 64-bit version
wine64 helloworld64

# run 32-bit version (LdrInitializeThunk Main exe initialization failed, 
status c0000017)
wine64 helloworld



On 17/10/18 8:12 pm, Alexandre Julliard wrote:
> Brendan McGrath <brendan(a)redmandi.com> writes:
> > 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.
> > wine64 should not be used for a 32-bit binary, it should exec the 32-bit
> > loader instead. Do you have a sample app to reproduce this?