Re: make check_no_exec() work reliable

[Peter Beutner]

Peter Beutner wrote:
> Before starting to make this whole noexecute override behaviour configurable,
> it first must work reliable. In its current form there is no guarantee that
> the check_no_exec() function is actually called, because any other installed
> exception handler might decide to handle the exception itself.
> And as seen by the number of failing applications, this seems to happen
> quite a lot.
> This patch therefore makes check_no_exec() to be called before any other
> exception handler.
>
> ---
> Any comments what is wrong with this one?

make check_no_exec() work reliable

[Alexandre Julliard]

Peter Beutner <p.beutner(a)gmx.net> writes:
> Peter Beutner wrote:
> > Before starting to make this whole noexecute override behaviour configurable,
> > it first must work reliable. In its current form there is no guarantee that
> > the check_no_exec() function is actually called, because any other installed
> > exception handler might decide to handle the exception itself.
> > And as seen by the number of failing applications, this seems to happen
> > quite a lot.
> > This patch therefore makes check_no_exec() to be called before any other
> > exception handler.
> >
> > ---
> > Any comments what is wrong with this one?

If the exception is not made visible to the app then there is no point
in having one at all, we might just as well turn off the protection
right away. What really needs to be done is to investigate the Windows
behavior and determine which parts of the app memory should be
protected and which shouldn't, depending on the exe flags etc. and
then replicate that behavior.

-- 
Alexandre Julliard
julliard(a)winehq.org

make check_no_exec() work reliable

[Peter Beutner]

Alexandre Julliard wrote:
> Peter Beutner <p.beutner(a)gmx.net> writes:
> > Peter Beutner wrote:
> > > Before starting to make this whole noexecute override behaviour configurable,
> > > it first must work reliable. In its current form there is no guarantee that
> > > the check_no_exec() function is actually called, because any other installed
> > > exception handler might decide to handle the exception itself.
> > > And as seen by the number of failing applications, this seems to happen
> > > quite a lot.
> > > This patch therefore makes check_no_exec() to be called before any other
> > > exception handler.
> > >
> > > ---
> > > Any comments what is wrong with this one?
>
> If the exception is not made visible to the app then there is no point
> in having one at all, we might just as well turn off the protection
> right away.
> Why should this exception be visible to the application?
> Plus if you make it visible, you can just forget this whole workaround idea,
> because it won't work reliable anyways.

And remembering the last discussion I thought "just turning off the protection"
wasn't your preferred solution either.
> What really needs to be done is to investigate the Windows
> behavior and determine which parts of the app memory should be
> protected and which shouldn't, depending on the exe flags etc. and
> then replicate that behavior.

Certainly some more testing on windows is needed, but this
check_no_exec() workaround was regardless introduced a long time ago.
This patch just fixes this workaround.
Otherwise it could as well be removed at all.

make check_no_exec() work reliable

[Alexandre Julliard]

Peter Beutner <p.beutner(a)gmx.net> writes:
> Why should this exception be visible to the application?
> Plus if you make it visible, you can just forget this whole workaround idea,
> because it won't work reliable anyways.

Well, yes, the workaround is really a hack that should be replaced by
a proper fix; I was hoping it would encourage someone to look into it
and fix it properly, I can't do it because my box doesn't have noexec
support.
> Certainly some more testing on windows is needed, but this
> check_no_exec() workaround was regardless introduced a long time ago.
> This patch just fixes this workaround.
> Otherwise it could as well be removed at all.

Well, yes, it could certainly be removed; I added it mostly to make
sure that we generated the exception properly, and to demonstrate how
the exception can be handled. The proper fix is clearly more complex
than that, but moving that hack into the exception code isn't a step
in the right direction IMO.

-- 
Alexandre Julliard
julliard(a)winehq.org

make check_no_exec() work reliable

[Peter Beutner]

Alexandre Julliard wrote:
> Peter Beutner <p.beutner(a)gmx.net> writes:
> > Why should this exception be visible to the application?
> > Plus if you make it visible, you can just forget this whole workaround idea,
> > because it won't work reliable anyways.
>
> Well, yes, the workaround is really a hack that should be replaced by
> a proper fix; I was hoping it would encourage someone to look into it
> and fix it properly, I can't do it because my box doesn't have noexec
> support.
> > Certainly some more testing on windows is needed, but this
> > check_no_exec() workaround was regardless introduced a long time ago.
> > This patch just fixes this workaround.
> > Otherwise it could as well be removed at all.
>
> Well, yes, it could certainly be removed; I added it mostly to make
> sure that we generated the exception properly, and to demonstrate how
> the exception can be handled. The proper fix is clearly more complex
> than that, but moving that hack into the exception code isn't a step
> in the right direction IMO.

hm, at least for that message box that was talked about, I don't see
another choice than to put it into the exception code.
But that probably should then only be the message box (and maybe a possibility
to change the noexecute configuration for this app) and no code for
trying to workaround the protection.

I also took a closer look on the windows behaviour.
There is no (at least application visible) difference in how the memory areas(heap,
stack,valloc,image) are mapped between NX disabled/enabled mode.
(Apart from the mysterious fact that a bunch of more dlls are loaded when it is disabled.)
The only difference is whether code runs from non-executable memory or not.
I'll attach the output of a small testcase I've written which shows the memory layout
for both cases.
 
There is however one exception. If the noexecute policy is set to OptOut[1] and windows
loads a PE file where the AddressEntryPoint is in a non-executable section,
NX protection is automatically switched off for this process.
According to some paper[2] there are even more specific compatibility checks, e.g. for
the safedisc driver. Sadly MS just speaks of "System compatibility fixes" without further 
detailed explanation :/

But as linux can't just switch on/off the protection for specific processes, wine has to
emulate it by marking all readable memory as executable as well. And as all this happens
behind the application's back, I would still go with my first proposal to just pair 
every PROT_READ with a PROT_EXEC in dlls/ntdll/virtual.c:VIRTUAL_GetUnixProt().

Does that sound acceptable?


1) means: apply protection for all apps, but allow to disable it for specific apps
2) http://www.uninformed.org/?v=2&a=4


run tests:
	execute on    stack: failed
	execute on  .rodata: failed
	execute on    .data: failed
	execute on     heap: failed
	execute on   valloc: failed
-----------------------------------------------
dump memory info:
0x00010000 - 0x00011fff -rw- private 
0x00020000 - 0x00020fff -rw- private 
0x0022d000 - 0x0022dfff grw- private 
0x0022e000 - 0x0022ffff -rw- private (stack)
0x00230000 - 0x00232fff -r-- mapped
0x00240000 - 0x00243fff -rw- private (heap)
0x00340000 - 0x00345fff -rw- private 
0x00350000 - 0x00352fff -rw- mapped
0x00360000 - 0x00375fff -r-- mapped
0x00380000 - 0x003bcfff -r-- mapped
0x003c0000 - 0x003c5fff -r-- mapped
0x003d0000 - 0x003d5fff -rw- private 
0x003e0000 - 0x003e2fff -r-- mapped
0x003f0000 - 0x003f0fff -rwe private 
0x00400000 - 0x00400fff -r-- image file: test_mem.exe (400000 - 408000)
0x00401000 - 0x00403fff -r-e image section:    .text flags: r-x CODE
0x00404000 - 0x00404fff -rw- image section:    .data flags: rw- DATA
0x00405000 - 0x00405fff -r-- image section:   .rdata flags: r-- DATA
0x00406000 - 0x00407fff -rw- image section:     .bss flags: rw- BSS
0x00410000 - 0x00450fff -r-- mapped
0x00460000 - 0x00460fff -rw- private (valloc)
0x77be0000 - 0x77be0fff -r-- image file: msvcrt.dll (77be0000 - 77c38000)
0x77be1000 - 0x77c2cfff -r-e image section:    .text flags: r-x CODE
0x77c2d000 - 0x77c2efff -rw- image section:    .data flags: rw- DATA
0x77c2f000 - 0x77c2ffff -rw- image section:    .data flags: rw- DATA
0x77c30000 - 0x77c30fff -rw- image section:    .data flags: rw- DATA
0x77c31000 - 0x77c33fff -rw- image section:    .data flags: rw- DATA
0x77c34000 - 0x77c37fff -r-- image section:    .rsrc flags: r-- DATA
0x7c800000 - 0x7c800fff -r-- image file: kernel32.dll (7c800000 - 7c906000)
0x7c801000 - 0x7c882fff -r-e image section:    .text flags: r-x CODE
0x7c883000 - 0x7c885fff -rw- image section:    .data flags: rw- DATA
0x7c886000 - 0x7c887fff -rw- image section:    .data flags: rw- DATA
0x7c888000 - 0x7c905fff -r-- image section:    .rsrc flags: r-- DATA
0x7c910000 - 0x7c910fff -r-- image file: ntdll.dll (7c910000 - 7c9c7000)
0x7c911000 - 0x7c98bfff -r-e image section:    .text flags: r-x CODE
0x7c98c000 - 0x7c98efff -rw- image section:    .data flags: rw- DATA
0x7c98f000 - 0x7c990fff -rw- image section:    .data flags: rw- DATA
0x7c991000 - 0x7c9c6fff -r-- image section:    .rsrc flags: r-- DATA
0x7f6f0000 - 0x7f6f6fff -r-e mapped
0x7ffb0000 - 0x7ffd3fff -r-- mapped
0x7ffdc000 - 0x7ffdcfff -rw- private 
0x7ffdf000 - 0x7ffdffff -rw- private 
0x7ffe0000 - 0x7ffe0fff -r-- private 


run tests:
	execute on    stack: succeeded
	execute on  .rodata: succeeded
	execute on    .data: succeeded
	execute on     heap: succeeded
	execute on   valloc: succeeded
-----------------------------------------------
dump memory info:
0x00010000 - 0x00011fff -rw- private 
0x00020000 - 0x00020fff -rw- private 
0x00030000 - 0x00036fff -rw- private 
0x0023c000 - 0x0023cfff grw- private 
0x0023d000 - 0x0023ffff -rw- private (stack)
0x00240000 - 0x00242fff -r-- mapped
0x00250000 - 0x00259fff -rw- private (heap)
0x00350000 - 0x00355fff -rw- private 
0x00360000 - 0x00362fff -rw- mapped
0x00370000 - 0x00385fff -r-- mapped
0x00390000 - 0x003ccfff -r-- mapped
0x003d0000 - 0x003d5fff -r-- mapped
0x003e0000 - 0x003e7fff -rw- private 
0x003f0000 - 0x003f3fff -rw- private 
0x00400000 - 0x00400fff -r-- image file: test_mem.exe (400000 - 408000)
0x00401000 - 0x00403fff -r-e image section:    .text flags: r-x CODE
0x00404000 - 0x00404fff -rw- image section:    .data flags: rw- DATA
0x00405000 - 0x00405fff -r-- image section:   .rdata flags: r-- DATA
0x00406000 - 0x00407fff -rw- image section:     .bss flags: rw- BSS
0x00410000 - 0x00450fff -r-- mapped
0x00460000 - 0x00461fff -r-e mapped
0x00520000 - 0x00521fff -r-e mapped
0x00530000 - 0x00530fff -rw- private 
0x00540000 - 0x00540fff -rw- private 
0x00550000 - 0x00551fff -r-- mapped
0x00560000 - 0x00563fff -rw- private 
0x00570000 - 0x00571fff -r-- mapped
0x00580000 - 0x00580fff -rwe private 
0x00590000 - 0x00592fff -r-- mapped
0x005a0000 - 0x005a2fff -rw- private 
0x005e0000 - 0x006e2fff -r-- mapped
0x006f0000 - 0x0073cfff -r-e mapped
0x009f0000 - 0x009f0fff -rw- private 
0x00a70000 - 0x00a70fff -rw- private (valloc)
0x5b0f0000 - 0x5b0f0fff -r-- image file: UxTheme.dll (5b0f0000 - 5b128000)
0x5b0f1000 - 0x5b120fff -r-e image section:    .text flags: r-x CODE
0x5b121000 - 0x5b121fff -rw- image section:    .data flags: rw- DATA
0x5b122000 - 0x5b127fff -r-- image section:    .rsrc flags: r-- DATA
0x5cf00000 - 0x5cf00fff -r-- image file: ShimEng.dll (5cf00000 - 5cf26000)
0x5cf01000 - 0x5cf0efff -r-e image section:    .text flags: r-x CODE
0x5cf0f000 - 0x5cf11fff -rw- image section:    .data flags: rw- DATA
0x5cf12000 - 0x5cf21fff -rw- image section:    .data flags: rw- DATA
0x5cf22000 - 0x5cf22fff -rw- image section:    .data flags: rw- DATA
0x5cf23000 - 0x5cf25fff -r-- image section:    .rsrc flags: r-- DATA
0x5d450000 - 0x5d450fff -r-- image file: comctl32.dll (5d450000 - 5d4e7000)
0x5d451000 - 0x5d4c0fff -r-e image section:    .text flags: r-x CODE
0x5d4c1000 - 0x5d4c2fff -rw- image section:    .data flags: rw- DATA
0x5d4c3000 - 0x5d4c3fff -rw- image section:    .data flags: rw- DATA
0x5d4c4000 - 0x5d4e6fff -r-- image section:    .rsrc flags: r-- DATA
0x6fd90000 - 0x6fd90fff -r-- image file: AcGenral.DLL (6fd90000 - 6ff5a000)
0x6fd91000 - 0x6fdc2fff -r-e image section:    .text flags: r-x CODE
0x6fdc3000 - 0x6fdc3fff -rw- image section:    .data flags: rw- DATA
0x6fdc4000 - 0x6fdc7fff -rw- image section:    .data flags: rw- DATA
0x6fdc8000 - 0x6fdc8fff -rw- image section:    .data flags: rw- DATA
0x6fdc9000 - 0x6fdc9fff -rw- image section:    .data flags: rw- DATA
0x6fdca000 - 0x6fdcbfff -rw- image section:    .data flags: rw- DATA
0x6fdcc000 - 0x6ff59fff -r-- image section:    .rsrc flags: r-- DATA
0x76620000 - 0x76620fff -r-- image file: USERENV.dll (76620000 - 766d5000)
0x76621000 - 0x766bffff -r-e image section:    .text flags: r-x CODE
0x766c0000 - 0x766c1fff -rw- image section:    .data flags: rw- DATA
0x766c2000 - 0x766d4fff -r-- image section:    .rsrc flags: r-- DATA
0x76af0000 - 0x76af0fff -r-- image file: WINMM.dll (76af0000 - 76b1e000)
0x76af1000 - 0x76b0ffff -r-e image section:    .text flags: r-x CODE
0x76b10000 - 0x76b10fff -rw- image section:    .data flags: rw- DATA
0x76b11000 - 0x76b11fff -rw- image section:    .data flags: rw- DATA
0x76b12000 - 0x76b1dfff -r-- image section:    .rsrc flags: r-- DATA
0x770f0000 - 0x770f0fff -r-- image file: OLEAUT32.dll (770f0000 - 7717c000)
0x770f1000 - 0x77171fff -r-e image section:    .text flags: r-x CODE
0x77172000 - 0x77172fff -rw- image section:    .data flags: rw- DATA
0x77173000 - 0x77174fff -rw- image section:    .data flags: rw- DATA
0x77175000 - 0x7717bfff -r-- image section:    .rsrc flags: r-- DATA
0x773a0000 - 0x773a0fff -r-- image file: comctl32.dll (773a0000 - 774a2000)
0x773a1000 - 0x77430fff -r-e image section:    .text flags: r-x CODE
0x77431000 - 0x77431fff -rw- image section:    .data flags: rw- DATA
0x77432000 - 0x774a1fff -r-- image section:    .rsrc flags: r-- DATA
0x774b0000 - 0x774b0fff -r-- image file: ole32.dll (774b0000 - 775ed000)
0x774b1000 - 0x775d5fff -r-e image section:    .text flags: r-x CODE
0x775d6000 - 0x775d6fff -rw- image section:    .data flags: rw- DATA
0x775d7000 - 0x775dcfff -rw- image section:    .data flags: rw- DATA
0x775dd000 - 0x775ecfff -r-- image section:    .rsrc flags: r-- DATA
0x77bb0000 - 0x77bb0fff -r-- image file: MSACM32.dll (77bb0000 - 77bc5000)
0x77bb1000 - 0x77bc0fff -r-e image section:    .text flags: r-x CODE
0x77bc1000 - 0x77bc1fff -rw- image section:    .data flags: rw- DATA
0x77bc2000 - 0x77bc4fff -r-- image section:    .rsrc flags: r-- DATA
0x77bd0000 - 0x77bd0fff -r-- image file: VERSION.dll (77bd0000 - 77bd8000)
0x77bd1000 - 0x77bd4fff -r-e image section:    .text flags: r-x CODE
0x77bd5000 - 0x77bd5fff -rw- image section:    .data flags: rw- DATA
0x77bd6000 - 0x77bd7fff -r-- image section:    .rsrc flags: r-- DATA
0x77be0000 - 0x77be0fff -r-- image file: msvcrt.dll (77be0000 - 77c38000)
0x77be1000 - 0x77c2cfff -r-e image section:    .text flags: r-x CODE
0x77c2d000 - 0x77c2efff -rw- image section:    .data flags: rw- DATA
0x77c2f000 - 0x77c2ffff -rw- image section:    .data flags: rw- DATA
0x77c30000 - 0x77c30fff -rw- image section:    .data flags: rw- DATA
0x77c31000 - 0x77c33fff -rw- image section:    .data flags: rw- DATA
0x77c34000 - 0x77c37fff -r-- image section:    .rsrc flags: r-- DATA
0x77d10000 - 0x77d10fff -r-- image file: USER32.dll (77d10000 - 77da0000)
0x77d11000 - 0x77d6ffff -r-e image section:    .text flags: r-x CODE
0x77d70000 - 0x77d70fff -rw- image section:    .data flags: rw- DATA
0x77d71000 - 0x77d71fff -rw- image section:    .data flags: rw- DATA
0x77d72000 - 0x77d9ffff -r-- image section:    .rsrc flags: r-- DATA
0x77da0000 - 0x77da0fff -r-- image file: ADVAPI32.dll (77da0000 - 77e4a000)
0x77da1000 - 0x77e15fff -r-e image section:    .text flags: r-x CODE
0x77e16000 - 0x77e16fff -rw- image section:    .data flags: rw- DATA
0x77e17000 - 0x77e1afff -rw- image section:    .data flags: rw- DATA
0x77e1b000 - 0x77e49fff -r-- image section:    .rsrc flags: r-- DATA
0x77e50000 - 0x77e50fff -r-- image file: RPCRT4.dll (77e50000 - 77ee1000)
0x77e51000 - 0x77ed9fff -r-e image section:    .text flags: r-x CODE
0x77eda000 - 0x77edafff -rw- image section:    .data flags: rw- DATA
0x77edb000 - 0x77ee0fff -r-- image section:    .rsrc flags: r-- DATA
0x77ef0000 - 0x77ef0fff -r-- image file: GDI32.dll (77ef0000 - 77f36000)
0x77ef1000 - 0x77f31fff -r-e image section:    .text flags: r-x CODE
0x77f32000 - 0x77f32fff -rw- image section:    .data flags: rw- DATA
0x77f33000 - 0x77f35fff -r-- image section:    .rsrc flags: r-- DATA
0x77f40000 - 0x77f40fff -r-- image file: SHLWAPI.dll (77f40000 - 77fb6000)
0x77f41000 - 0x77facfff -r-e image section:    .text flags: r-x CODE
0x77fad000 - 0x77fadfff -rw- image section:    .data flags: rw- DATA
0x77fae000 - 0x77fb5fff -r-- image section:    .rsrc flags: r-- DATA
0x7c800000 - 0x7c800fff -r-- image file: kernel32.dll (7c800000 - 7c906000)
0x7c801000 - 0x7c882fff -r-e image section:    .text flags: r-x CODE
0x7c883000 - 0x7c885fff -rw- image section:    .data flags: rw- DATA
0x7c886000 - 0x7c887fff -rw- image section:    .data flags: rw- DATA
0x7c888000 - 0x7c905fff -r-- image section:    .rsrc flags: r-- DATA
0x7c910000 - 0x7c910fff -r-- image file: ntdll.dll (7c910000 - 7c9c7000)
0x7c911000 - 0x7c98bfff -r-e image section:    .text flags: r-x CODE
0x7c98c000 - 0x7c98efff -rw- image section:    .data flags: rw- DATA
0x7c98f000 - 0x7c990fff -rw- image section:    .data flags: rw- DATA
0x7c991000 - 0x7c9c6fff -r-- image section:    .rsrc flags: r-- DATA
0x7c9d0000 - 0x7c9d0fff -r-- image file: SHELL32.dll (7c9d0000 - 7d1ee000)
0x7c9d1000 - 0x7cbcbfff -r-e image section:    .text flags: r-x CODE
0x7cbcc000 - 0x7cbdbfff -rw- image section:    .data flags: rw- DATA
0x7cbdc000 - 0x7cbe1fff -rw- image section:    .data flags: rw- DATA
0x7cbe2000 - 0x7cbe8fff -rw- image section:    .data flags: rw- DATA
0x7cbe9000 - 0x7d1edfff -r-- image section:    .rsrc flags: r-- DATA
0x7f6f0000 - 0x7f6f6fff -r-e mapped
0x7ffb0000 - 0x7ffd3fff -r-- mapped
0x7ffdc000 - 0x7ffdcfff -rw- private 
0x7ffdf000 - 0x7ffdffff -rw- private 
0x7ffe0000 - 0x7ffe0fff -r-- private

make check_no_exec() work reliable

[Alexandre Julliard]

Peter Beutner <p.beutner(a)gmx.net> writes:
> But as linux can't just switch on/off the protection for specific processes, wine has to
> emulate it by marking all readable memory as executable as well. And as all this happens
> behind the application's back, I would still go with my first proposal to just pair 
> every PROT_READ with a PROT_EXEC in dlls/ntdll/virtual.c:VIRTUAL_GetUnixProt().
>
> Does that sound acceptable?

Given your investigations that sounds reasonable, yes, thanks for
taking the time to look into this. Does something like this work for
you?

diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
index 99c90c4..4d15779 100644
--- a/dlls/ntdll/loader.c
+++ b/dlls/ntdll/loader.c
@@ -2126,6 +2126,8 @@ void WINAPI LdrInitializeThunk( ULONG unknown1, ULONG unknown2, ULONG unknown3,
 
     peb->ProcessParameters->ImagePathName = wm->ldr.FullDllName;
     version_init( wm->ldr.FullDllName.Buffer );
+    if (!(nt->OptionalHeader.DllCharacteristics & IMAGE_DLLCHARACTERISTICS_NX_COMPAT))
+        VIRTUAL_SetForceExec( TRUE );
 
     /* the main exe needs to be the first in the load order list */
     RemoveEntryList( &wm->ldr.InLoadOrderModuleList );
diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h
index 781afa5..a7ef8df 100644
--- a/dlls/ntdll/ntdll_misc.h
+++ b/dlls/ntdll/ntdll_misc.h
@@ -112,6 +112,7 @@ extern NTSTATUS DIR_get_unix_cwd( char **cwd );
 /* virtual memory */
 extern NTSTATUS VIRTUAL_HandleFault(LPCVOID addr);
 extern BOOL VIRTUAL_HasMapping( LPCVOID addr );
+extern void VIRTUAL_SetForceExec( BOOL enable );
 extern void VIRTUAL_UseLargeAddressSpace(void);
 
 extern BOOL is_current_process( HANDLE handle );
diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
index 573072e..5da929b 100644
--- a/dlls/ntdll/virtual.c
+++ b/dlls/ntdll/virtual.c
@@ -143,6 +143,9 @@ static void *user_space_limit = USER_SPACE_LIMIT;
 static void *preload_reserve_start;
 static void *preload_reserve_end;
 static int use_locks;
+static int force_exec_prot;  /* whether to force PROT_EXEC on all PROT_READ mmaps */
+
+static int VIRTUAL_GetUnixProt( BYTE vprot );
 
 
 /***********************************************************************
@@ -391,6 +394,7 @@ static NTSTATUS create_view( struct file_view **view_ret, void *base, size_t siz
 {
     struct file_view *view;
     struct list *ptr;
+    int unix_prot = VIRTUAL_GetUnixProt( vprot );
 
     assert( !((UINT_PTR)base & page_mask) );
     assert( !(size & page_mask) );
@@ -446,6 +450,12 @@ static NTSTATUS create_view( struct file_view **view_ret, void *base, size_t siz
 
     *view_ret = view;
     VIRTUAL_DEBUG_DUMP_VIEW( view );
+
+    if (force_exec_prot && (unix_prot & PROT_READ) && !(unix_prot & PROT_EXEC))
+    {
+        TRACE( "forcing exec permission on %p-%p\n", base, (char *)base + size - 1 );
+        mprotect( base, size, unix_prot | PROT_EXEC );
+    }
     return STATUS_SUCCESS;
 }
 
@@ -555,12 +565,22 @@ static BOOL VIRTUAL_SetProt( FILE_VIEW *view, /* [in] Pointer to view */
                              size_t size,     /* [in] Size in bytes */
                              BYTE vprot )     /* [in] Protections to use */
 {
+    int unix_prot = VIRTUAL_GetUnixProt(vprot);
+
     TRACE("%p-%p %s\n",
           base, (char *)base + size - 1, VIRTUAL_GetProtStr( vprot ) );
 
-    if (mprotect( base, size, VIRTUAL_GetUnixProt(vprot) ))
-        return FALSE;  /* FIXME: last error */
+    if (force_exec_prot && (unix_prot & PROT_READ) && !(unix_prot & PROT_EXEC))
+    {
+        TRACE( "forcing exec permission on %p-%p\n", base, (char *)base + size - 1 );
+        if (!mprotect( base, size, unix_prot | PROT_EXEC )) goto done;
+        /* exec + write may legitimately fail, in that case fall back to write only */
+        if (!(unix_prot & PROT_WRITE)) return FALSE;
+    }
 
+    if (mprotect( base, size, unix_prot )) return FALSE;  /* FIXME: last error */
+
+done:
     memset( view->prot + (((char *)base - (char *)view->base) >> page_shift),
             vprot, size >> page_shift );
     VIRTUAL_DEBUG_DUMP_VIEW( view );
@@ -1285,6 +1305,61 @@ BOOL VIRTUAL_HasMapping( LPCVOID addr )
 
 
 /***********************************************************************
+ *           VIRTUAL_SetForceExec
+ *
+ * Whether to force exec prot on all views.
+ */
+void VIRTUAL_SetForceExec( BOOL enable )
+{
+    struct file_view *view;
+
+    RtlEnterCriticalSection( &csVirtual );
+    if (!force_exec_prot != !enable)  /* change all existing views */
+    {
+        force_exec_prot = enable;
+
+        LIST_FOR_EACH_ENTRY( view, &views_list, struct file_view, entry )
+        {
+            UINT i, count;
+            int unix_prot;
+            char *addr = view->base;
+            BYTE prot = view->prot[0];
+
+            for (count = i = 1; i < view->size >> page_shift; i++, count++)
+            {
+                if (view->prot[i] == prot) continue;
+                unix_prot = VIRTUAL_GetUnixProt( prot );
+                if ((unix_prot & PROT_READ) && !(unix_prot & PROT_EXEC))
+                {
+                    TRACE( "%s exec prot for %p-%p\n",
+                           force_exec_prot ? "enabling" : "disabling",
+                           addr, addr + (count << page_shift) - 1 );
+                    mprotect( addr, count << page_shift,
+                              unix_prot | (force_exec_prot ? PROT_EXEC : 0) );
+                }
+                addr += (count << page_shift);
+                prot = view->prot[i];
+                count = 0;
+            }
+            if (count)
+            {
+                unix_prot = VIRTUAL_GetUnixProt( prot );
+                if ((unix_prot & PROT_READ) && !(unix_prot & PROT_EXEC))
+                {
+                    TRACE( "%s exec prot for %p-%p\n",
+                           force_exec_prot ? "enabling" : "disabling",
+                           addr, addr + (count << page_shift) - 1 );
+                    mprotect( addr, count << page_shift,
+                              unix_prot | (force_exec_prot ? PROT_EXEC : 0) );
+                }
+            }
+        }
+    }
+    RtlLeaveCriticalSection( &csVirtual );
+}
+
+
+/***********************************************************************
  *           VIRTUAL_UseLargeAddressSpace
  *
  * Increase the address space size for apps that support it.

-- 
Alexandre Julliard
julliard(a)winehq.org

make check_no_exec() work reliable

[Peter Beutner]

Alexandre Julliard wrote:
> Peter Beutner <p.beutner(a)gmx.net> writes:
> > But as linux can't just switch on/off the protection for specific processes, wine has to
> > emulate it by marking all readable memory as executable as well. And as all this happens
> > behind the application's back, I would still go with my first proposal to just pair 
> > every PROT_READ with a PROT_EXEC in dlls/ntdll/virtual.c:VIRTUAL_GetUnixProt().
> >
> > Does that sound acceptable?
>
> Given your investigations that sounds reasonable, yes, thanks for
> taking the time to look into this. Does something like this work for
> you?

Yes, it works. This should be pretty much equal to the default OptIn policy under windows,
i.e. disable the protection for everyone unless the app explicitely say it can handle it.
(except for the fact that wine always does this now, even if the cpu hasn't NX or something
aquivalent.Shouldn't really hurt anyone though.)
> diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
> index 99c90c4..4d15779 100644
> --- a/dlls/ntdll/loader.c
> +++ b/dlls/ntdll/loader.c
> @@ -2126,6 +2126,8 @@ void WINAPI LdrInitializeThunk( ULONG unknown1, ULONG unknown2, ULONG unknown3,
>
> peb->ProcessParameters->ImagePathName = wm->ldr.FullDllName;
>      version_init( wm->ldr.FullDllName.Buffer );
> +    if (!(nt->OptionalHeader.DllCharacteristics & IMAGE_DLLCHARACTERISTICS_NX_COMPAT))
> +        VIRTUAL_SetForceExec( TRUE );

I'm not so sure about this, I think I read somewhere that each loaded DLL is checked for this
not just the main exe and the protection is disabled if at least on module is not nx compatible.
But as no wine dll is marked as NX_COMPAT(i assume) this would basically mean to always disable the
protection.
It probably doesn't hurt to leave it like this for now.
The rest (build wine dlls with NX_COMPAT and do this check for every lib) can still be added later.
I think it's not quite common anyway that the main exe is flagged as NX_COMPAT and some dll of the
app is not ;)

make check_no_exec() work reliable

[Alexandre Julliard]

Peter Beutner <p.beutner(a)gmx.net> writes:
> I'm not so sure about this, I think I read somewhere that each loaded DLL is checked for this
> not just the main exe and the protection is disabled if at least on module is not nx compatible.
> But as no wine dll is marked as NX_COMPAT(i assume) this would basically mean to always disable the
> protection.
> It probably doesn't hurt to leave it like this for now.

Yes, the next step is to implement a smarter strategy for turning
protection on or off. I don't know about checking the flag for all
dlls or only the main exe, I guess I'll have to ask you to try this on
Windows too...

-- 
Alexandre Julliard
julliard(a)winehq.org