[PATCH v2 0/3] MR260: ntdll: Improve xstate validation in NtGetContextThread()
Supposed to fix SCP: Secret Laboratory intermittent crash at main menu. The first two patches make the existing validation also work for other thread context query (as context_from_server called through get_thread_context() doesn't do that). The last patch adds xstate alignment validation with corresponding test. -- v2: ntdll: Validate xstate alignment in validate_context_xstate(). ntdll: Validate context xstate at once in NtGetContextThread(). ntdll: Factor out validate_context_xstate() function. https://gitlab.winehq.org/wine/wine/-/merge_requests/260
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/signal_i386.c | 4 +--- dlls/ntdll/unix/signal_x86_64.c | 4 +--- dlls/ntdll/unix/thread.c | 17 +++++++++++++++++ dlls/ntdll/unix/unix_private.h | 1 + 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index 7be0c39c424..a515b21cc2e 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -1098,9 +1098,7 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) XSTATE *xstate = (XSTATE *)((char *)context_ex + context_ex->XState.Offset); unsigned int mask; - if (context_ex->XState.Length < offsetof(XSTATE, YmmContext) - || context_ex->XState.Length > sizeof(XSTATE)) - return STATUS_INVALID_PARAMETER; + if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; mask = (xstate_compaction_enabled ? xstate->CompactionMask : xstate->Mask) & XSTATE_MASK_GSSE; xstate->Mask = frame->xstate.Mask & mask; diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 6c87e347eac..3aac1bc3e75 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1918,9 +1918,7 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) XSTATE *xstate = (XSTATE *)((char *)context_ex + context_ex->XState.Offset); unsigned int mask; - if (context_ex->XState.Length < offsetof(XSTATE, YmmContext) - || context_ex->XState.Length > sizeof(XSTATE)) - return STATUS_INVALID_PARAMETER; + if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; mask = (xstate_compaction_enabled ? xstate->CompactionMask : xstate->Mask) & XSTATE_MASK_GSSE; xstate->Mask = frame->xstate.Mask & mask; diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index ad47a5fce74..6289b8eadca 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -152,6 +152,23 @@ void fpu_to_fpux( XMM_SAVE_AREA32 *fpux, const I386_FLOATING_SAVE_AREA *fpu ) } +/*********************************************************************** + * validate_context_xstate + */ +BOOL validate_context_xstate( CONTEXT *context ) +{ + CONTEXT_EX *context_ex; + + context_ex = (CONTEXT_EX *)(context + 1); + + if (context_ex->XState.Length < offsetof(XSTATE, YmmContext) + || context_ex->XState.Length > sizeof(XSTATE)) + return FALSE; + + return TRUE; +} + + /*********************************************************************** * get_server_context_flags */ diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 795fc148479..5e101bbe363 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -185,6 +185,7 @@ extern void DECLSPEC_NORETURN abort_process( int status ) DECLSPEC_HIDDEN; extern void DECLSPEC_NORETURN exit_process( int status ) DECLSPEC_HIDDEN; extern void wait_suspend( CONTEXT *context ) DECLSPEC_HIDDEN; extern NTSTATUS send_debug_event( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL first_chance ) DECLSPEC_HIDDEN; +extern BOOL validate_context_xstate( CONTEXT *context ) DECLSPEC_HIDDEN; extern NTSTATUS set_thread_context( HANDLE handle, const void *context, BOOL *self, USHORT machine ) DECLSPEC_HIDDEN; extern NTSTATUS get_thread_context( HANDLE handle, void *context, BOOL *self, USHORT machine ) DECLSPEC_HIDDEN; extern NTSTATUS alloc_object_attributes( const OBJECT_ATTRIBUTES *attr, struct object_attributes **ret, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/260
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/unix/signal_i386.c | 4 ++-- dlls/ntdll/unix/signal_x86_64.c | 4 ++-- dlls/ntdll/unix/thread.c | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/signal_i386.c b/dlls/ntdll/unix/signal_i386.c index a515b21cc2e..95e7a69a6e1 100644 --- a/dlls/ntdll/unix/signal_i386.c +++ b/dlls/ntdll/unix/signal_i386.c @@ -1003,6 +1003,8 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) BOOL self = (handle == GetCurrentThread()); NTSTATUS ret; + if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; + /* debug registers require a server call */ if (needed_flags & CONTEXT_DEBUG_REGISTERS) self = FALSE; @@ -1098,8 +1100,6 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) XSTATE *xstate = (XSTATE *)((char *)context_ex + context_ex->XState.Offset); unsigned int mask; - if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; - mask = (xstate_compaction_enabled ? xstate->CompactionMask : xstate->Mask) & XSTATE_MASK_GSSE; xstate->Mask = frame->xstate.Mask & mask; xstate->CompactionMask = xstate_compaction_enabled ? (0x8000000000000000 | mask) : 0; diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c index 3aac1bc3e75..11e652e1dca 100644 --- a/dlls/ntdll/unix/signal_x86_64.c +++ b/dlls/ntdll/unix/signal_x86_64.c @@ -1832,6 +1832,8 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) DWORD needed_flags = context->ContextFlags & ~CONTEXT_AMD64; BOOL self = (handle == GetCurrentThread()); + if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; + /* debug registers require a server call */ if (needed_flags & CONTEXT_DEBUG_REGISTERS) self = FALSE; @@ -1918,8 +1920,6 @@ NTSTATUS WINAPI NtGetContextThread( HANDLE handle, CONTEXT *context ) XSTATE *xstate = (XSTATE *)((char *)context_ex + context_ex->XState.Offset); unsigned int mask; - if (!validate_context_xstate( context )) return STATUS_INVALID_PARAMETER; - mask = (xstate_compaction_enabled ? xstate->CompactionMask : xstate->Mask) & XSTATE_MASK_GSSE; xstate->Mask = frame->xstate.Mask & mask; xstate->CompactionMask = xstate_compaction_enabled ? (0x8000000000000000 | mask) : 0; diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index 6289b8eadca..b393315e6fe 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -159,6 +159,8 @@ BOOL validate_context_xstate( CONTEXT *context ) { CONTEXT_EX *context_ex; + if (!((context->ContextFlags & 0x40) && (cpu_info.ProcessorFeatureBits & CPU_FEATURE_AVX))) return TRUE; + context_ex = (CONTEXT_EX *)(context + 1); if (context_ex->XState.Length < offsetof(XSTATE, YmmContext) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/260
From: Paul Gofman <pgofman(a)codeweavers.com> Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/tests/exception.c | 14 ++++++++++++++ dlls/ntdll/unix/thread.c | 2 ++ 2 files changed, 16 insertions(+) diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c index 820e435bc1b..06a38756f80 100644 --- a/dlls/ntdll/tests/exception.c +++ b/dlls/ntdll/tests/exception.c @@ -9445,6 +9445,7 @@ static void test_extended_context(void) CONTEXT_EX *context_ex; CONTEXT *context; unsigned data[8]; + NTSTATUS status; HANDLE thread; ULONG64 mask; XSTATE *xs; @@ -10227,6 +10228,19 @@ static void test_extended_context(void) thread = CreateThread(NULL, 0, test_extended_context_thread, 0, CREATE_SUSPENDED, NULL); ok(!!thread, "Failed to create thread.\n"); + /* Unaligned xstate. */ + length = sizeof(context_buffer); + memset(context_buffer, 0xcc, sizeof(context_buffer)); + bret = pInitializeContext(context_buffer, CONTEXT_FULL | CONTEXT_XSTATE | CONTEXT_FLOATING_POINT, + &context, &length); + ok(bret, "Got unexpected bret %#x.\n", bret); + context_ex = (CONTEXT_EX *)(context + 1); + context_ex->XState.Offset += 0x10; + status = pNtGetContextThread(thread, context); + ok(status == STATUS_INVALID_PARAMETER, "Unexpected status %#lx.\n", status); + status = pNtGetContextThread(GetCurrentThread(), context); + ok(status == STATUS_INVALID_PARAMETER, "Unexpected status %#lx.\n", status); + bret = pInitializeContext(context_buffer, CONTEXT_FULL | CONTEXT_XSTATE | CONTEXT_FLOATING_POINT, &context, &length); ok(bret, "Got unexpected bret %#x.\n", bret); diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index b393315e6fe..00ee607a671 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -167,6 +167,8 @@ BOOL validate_context_xstate( CONTEXT *context ) || context_ex->XState.Length > sizeof(XSTATE)) return FALSE; + if (((ULONG_PTR)context_ex + context_ex->XState.Offset) & 63) return FALSE; + return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/260
This merge request was closed by Paul Gofman. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/260
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)