[PATCH 2/2 v2] ntdll: Track copy-on-write memory state on i386 and x86_64 architectures.
Based on patch by Michael Müller.
Signed-off-by: Piotr Caban piotr@codeweavers.com --- v2: - added credit for Michael
dlls/kernel32/tests/virtual.c | 12 +++----- dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/signal_arm.c | 8 +++++ dlls/ntdll/signal_arm64.c | 8 +++++ dlls/ntdll/signal_i386.c | 51 +++++++++++++++++++++++++++++++ dlls/ntdll/signal_powerpc.c | 8 +++++ dlls/ntdll/signal_x86_64.c | 45 +++++++++++++++++++++++++++ dlls/ntdll/thread.c | 1 + dlls/ntdll/virtual.c | 57 +++++++++++++++++++++++++++-------- 9 files changed, 170 insertions(+), 21 deletions(-)
Am Fr., 22. Feb. 2019 um 16:48 Uhr schrieb Piotr Caban piotr@codeweavers.com:
Based on patch by Michael Müller.
Signed-off-by: Piotr Caban piotr@codeweavers.com
v2:
- added credit for Michael
dlls/kernel32/tests/virtual.c | 12 +++----- dlls/ntdll/ntdll_misc.h | 1 + dlls/ntdll/signal_arm.c | 8 +++++ dlls/ntdll/signal_arm64.c | 8 +++++ dlls/ntdll/signal_i386.c | 51 +++++++++++++++++++++++++++++++ dlls/ntdll/signal_powerpc.c | 8 +++++ dlls/ntdll/signal_x86_64.c | 45 +++++++++++++++++++++++++++ dlls/ntdll/thread.c | 1 + dlls/ntdll/virtual.c | 57 +++++++++++++++++++++++++++-------- 9 files changed, 170 insertions(+), 21 deletions(-)
Hello Piotr,
I didn't fully review your patch, but note that we had this feature basically disabled in Staging (hidden behind an environment variable) because it caused a lot of trouble.
The main issues were:
* Syscalls will just fail with EFAULT when they encounter a page without sufficient protections. It will not trigger a signal! This means it would be necessary to add code to handle EFAULT whenever there is a chance that the memory passed by the user might have the copy-on-write flag. In particular, this affects all wineserver calls which directly write to user-provided buffers. See:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
* For third party libraries you always have to ensure that faults are handled before passing any pointer. This even affects the OpenGL libs: They pass memory addresses directly to the kernel, and thus don't trigger the write patches. We noticed weird rendering errors in several games with the copy-on-write logic enabled.
Just to make sure, can those issues still happen in your version of the patch?
Best regards, Sebastian
Hi Sebastian,
On 2/22/19 5:08 PM, Sebastian Lackner wrote:
I didn't fully review your patch, but note that we had this feature basically disabled in Staging (hidden behind an environment variable) because it caused a lot of trouble.
The main issues were:
- Syscalls will just fail with EFAULT when they encounter a page
without sufficient protections. It will not trigger a signal! This means it would be necessary to add code to handle EFAULT whenever there is a chance that the memory passed by the user might have the copy-on-write flag. In particular, this affects all wineserver calls which directly write to user-provided buffers. See:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
The code is changed in a way so it behaves exactly the same as memory with write watch. Before executing the syscall check_write_access should make the memory readable. I think copy-on-write EFAULT shouldn't happen during syscall in current wine.
- For third party libraries you always have to ensure that faults are
handled before passing any pointer. This even affects the OpenGL libs: They pass memory addresses directly to the kernel, and thus don't trigger the write patches. We noticed weird rendering errors in several games with the copy-on-write logic enabled.
Do you remember any of the games that were affected? I'm expecting it to still be a problem.
Thanks, Piotr
Hello Piotr,
On 22.02.19 17:38, Piotr Caban wrote:
Hi Sebastian,
On 2/22/19 5:08 PM, Sebastian Lackner wrote:
I didn't fully review your patch, but note that we had this feature basically disabled in Staging (hidden behind an environment variable) because it caused a lot of trouble.
The main issues were:
- Syscalls will just fail with EFAULT when they encounter a page
without sufficient protections. It will not trigger a signal! This means it would be necessary to add code to handle EFAULT whenever there is a chance that the memory passed by the user might have the copy-on-write flag. In particular, this affects all wineserver calls which directly write to user-provided buffers. See:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
The code is changed in a way so it behaves exactly the same as memory with write watch. Before executing the syscall check_write_access should make the memory readable. I think copy-on-write EFAULT shouldn't happen during syscall in current wine.
When check_write_access() is called everything should be fine, but it doesn't look like it is used everywhere. What about all the NtQuery*() functions that pass through user-provided pointers? See for example NtQueryInformationAtom or RtlQueryAtomInAtomTable - both directly pass an untrusted pointer to wine_server_set_reply() and both use wine_server_call() instead of any of the locked variants. Also, there still seems to be at least one plain read() call in advapi32:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
- For third party libraries you always have to ensure that faults are
handled before passing any pointer. This even affects the OpenGL libs: They pass memory addresses directly to the kernel, and thus don't trigger the write patches. We noticed weird rendering errors in several games with the copy-on-write logic enabled.
Do you remember any of the games that were affected? I'm expecting it to still be a problem.
Unfortunately I am not sure anymore which game showed the graphic issues, but I would suspect that even a simple OpenGL sample code to download a texture into WRITECOPY memory would be affected. There were at least two bug reports related to WRITECOPY regressions though:
* https://bugs.winehq.org/show_bug.cgi?id=39349 * https://bugs.winehq.org/show_bug.cgi?id=39350
For the second one the app has probably changed in the meantime, but maybe the first one can still be used to reproduce one of the issues.
Best regards, Sebastian
Hi Sebastian,
On 2/22/19 8:24 PM, Sebastian Lackner wrote:
When check_write_access() is called everything should be fine, but it doesn't look like it is used everywhere. What about all the NtQuery*() functions that pass through user-provided pointers? See for example NtQueryInformationAtom or RtlQueryAtomInAtomTable - both directly pass an untrusted pointer to wine_server_set_reply() and both use wine_server_call() instead of any of the locked variants. Also, there still seems to be at least one plain read() call in advapi32:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
In all of these cases it will behave in the same way as if memory with write watch is passed. I guess that the idea was to fix it when we find an application that depends on that. In case of server calls it's easy to debug the problem. If it's preferred to change this calls before adding copy-on-write code I can start with that.
- For third party libraries you always have to ensure that faults are
handled before passing any pointer. This even affects the OpenGL libs: They pass memory addresses directly to the kernel, and thus don't trigger the write patches. We noticed weird rendering errors in several games with the copy-on-write logic enabled.
Do you remember any of the games that were affected? I'm expecting it to still be a problem.
Unfortunately I am not sure anymore which game showed the graphic issues, but I would suspect that even a simple OpenGL sample code to download a texture into WRITECOPY memory would be affected. There were at least two bug reports related to WRITECOPY regressions though:
For the second one the app has probably changed in the meantime, but maybe the first one can still be used to reproduce one of the issues.
I've done some testing regarding it. The opengl calls I've tested are not passing the memory to kernel. I've checked it only on following functions: glGenTextures and glGetTexImage. I guess it may depend on the driver or a better test needs to be written.
I've also looked on the 2 bugs you wrote about. In case of Talisman game it works for me. I didn't check what fixed it or if I can reproduce the problem with older wine. In case of Alone in the Dark there's a race in old version of game. The patches are just making the race much more visible.
If it turns out that there's a game that passes copy-on-write memory to opengl->kernel we will need to add e.g. IsBadWritePtr calls before the buffer is passed to opengl. I don't know if we want to do it before we find an application/graphics driver that depends on that.
Thanks, Piotr
Am Mi., 27. Feb. 2019 um 14:44 Uhr schrieb Piotr Caban piotr.caban@gmail.com:
Hi Sebastian,
On 2/22/19 8:24 PM, Sebastian Lackner wrote:
When check_write_access() is called everything should be fine, but it doesn't look like it is used everywhere. What about all the NtQuery*() functions that pass through user-provided pointers? See for example NtQueryInformationAtom or RtlQueryAtomInAtomTable - both directly pass an untrusted pointer to wine_server_set_reply() and both use wine_server_call() instead of any of the locked variants. Also, there still seems to be at least one plain read() call in advapi32:
https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-WRITE...
In all of these cases it will behave in the same way as if memory with write watch is passed. I guess that the idea was to fix it when we find an application that depends on that. In case of server calls it's easy to debug the problem. If it's preferred to change this calls before adding copy-on-write code I can start with that.
What I consider the main difference here is that write watches are only rarely used (only very few apps come to my mind), but write-copy protection is the default for many sections in PE applications - see map_image() in ntdll/virtual.c. In my opinion, there is a high chance that this change causes regressions (in particular, when there are still so many wineserver calls that don't handle write watches / write-copy protection), but my intention is not to block your changes. I only wanted to make you aware of the possible issues we experienced back then.
Best regards, Sebastian
-
Piotr Caban -
Piotr Caban -
Sebastian Lackner