From 68b69bd550bb94dc9ab958bac508215d849e029e Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 1 May 2024 00:47:49 +0200 Subject: [PATCH] Fix nonvolatile context restoration (#101709) * Fix nonvolatile context restoration There is a possibility of a race between the ClrRestoreNonVolatileContext and an async signal handling (like the one we use for runtime suspension). If the signal kicks in after we've loaded Rsp, but before we jumped to the target address, the context we are loading the registers from could get overwritten by the signal handler stack. So the ClrRestoreNonVolatileContext would end up jumping into a wrong target address. The fix is to load the target address into a register before loading the Rsp and then jumping using the register. * Fix arm and x86 --- src/coreclr/pal/src/arch/arm/context2.S | 21 ++++++++++++--------- src/coreclr/pal/src/arch/i386/context2.S | 17 ++++++++++++----- src/coreclr/vm/amd64/Context.S | 3 ++- src/coreclr/vm/amd64/Context.asm | 3 ++- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/coreclr/pal/src/arch/arm/context2.S b/src/coreclr/pal/src/arch/arm/context2.S index e292ca26fe2adc..bb41b0ff589b40 100644 --- a/src/coreclr/pal/src/arch/arm/context2.S +++ b/src/coreclr/pal/src/arch/arm/context2.S @@ -160,25 +160,28 @@ LOCAL_LABEL(Restore_CONTEXT_FLOATING_POINT): ldr R2, [r0, #(CONTEXT_Cpsr)] msr APSR, r2 - // Ideally, we would like to use `ldmia r0, {r0-r12, sp, lr, pc}` here, - // but clang 3.6 and later, as per ARM recommendation, disallows using - // Sp in the register list, and Pc and Lr simultaneously. - // So we are going to use the IPC register r12 to copy Sp, Lr and Pc - // which should be ok -- TODO: Is this really ok? + ldr r1, [r0, #(CONTEXT_Sp)] + ldr r2, [r0, #(CONTEXT_Pc)] + str r2, [r1, #-4] + ldr r2, [r0, #(CONTEXT_R12)] + str r2, [r1, #-8] add r12, r0, CONTEXT_R0 ldm r12, {r0-r11} - ldr sp, [r12, #(CONTEXT_Sp - (CONTEXT_R0))] ldr lr, [r12, #(CONTEXT_Lr - (CONTEXT_R0))] - ldr pc, [r12, #(CONTEXT_Pc - (CONTEXT_R0))] + ldr r12, [r12, #(CONTEXT_Sp - (CONTEXT_R0))] + sub r12, r12, #8 + mov sp, r12 + pop {r12, pc} LOCAL_LABEL(No_Restore_CONTEXT_INTEGER): ldr r2, [r0, #(CONTEXT_Cpsr)] msr APSR, r2 - ldr sp, [r0, #(CONTEXT_Sp)] ldr lr, [r0, #(CONTEXT_Lr)] - ldr pc, [r0, #(CONTEXT_Pc)] + ldr r2, [r0, #(CONTEXT_Pc)] + ldr sp, [r0, #(CONTEXT_Sp)] + bx r2 LOCAL_LABEL(No_Restore_CONTEXT_CONTROL): ldr r2, [r0, #(CONTEXT_ContextFlags)] diff --git a/src/coreclr/pal/src/arch/i386/context2.S b/src/coreclr/pal/src/arch/i386/context2.S index cf5b464c27d0ce..35768ea6232af2 100644 --- a/src/coreclr/pal/src/arch/i386/context2.S +++ b/src/coreclr/pal/src/arch/i386/context2.S @@ -115,11 +115,12 @@ LOCAL_LABEL(Done_Restore_CONTEXT_FLOATING_POINT): movdqu xmm7, [eax + CONTEXT_Xmm7] LOCAL_LABEL(Done_Restore_CONTEXT_EXTENDED_REGISTERS): - // Restore Stack - mov esp, [eax + CONTEXT_Esp] - // Create a minimal frame - push DWORD PTR [eax + CONTEXT_Eip] + mov ebx, [eax + CONTEXT_Esp] + mov ecx, [eax + CONTEXT_Eip] + mov edx, [eax + CONTEXT_Eax] + mov [ebx - 4], ecx + mov [ebx - 8], edx // Restore register(s) mov ebp, [eax + CONTEXT_Ebp] @@ -128,7 +129,13 @@ LOCAL_LABEL(Done_Restore_CONTEXT_EXTENDED_REGISTERS): mov edx, [eax + CONTEXT_Edx] mov ecx, [eax + CONTEXT_Ecx] mov ebx, [eax + CONTEXT_Ebx] - mov eax, [eax + CONTEXT_Eax] + + // Restore Stack + mov eax, [eax + CONTEXT_Esp] + sub eax, 8 + mov esp, eax + + pop eax // Resume ret diff --git a/src/coreclr/vm/amd64/Context.S b/src/coreclr/vm/amd64/Context.S index 0721c45f56727c..1eda47f0f9dc3f 100644 --- a/src/coreclr/vm/amd64/Context.S +++ b/src/coreclr/vm/amd64/Context.S @@ -51,8 +51,9 @@ NESTED_ENTRY ClrRestoreNonvolatileContextWorker, _TEXT, NoHandler // exception handling, iret and ret can't be used because their shadow stack enforcement would not allow that transition, // and using them would require writing to the shadow stack, which is not preferable. Instead, iret is partially // simulated. + mov rax, [r10 + OFFSETOF__CONTEXT__Rip] mov rsp, [r10 + OFFSETOF__CONTEXT__Rsp] - jmp qword ptr [r10 + OFFSETOF__CONTEXT__Rip] + jmp rax Done_Restore_CONTEXT_CONTROL: // The function was not asked to restore the control registers so we return back to the caller diff --git a/src/coreclr/vm/amd64/Context.asm b/src/coreclr/vm/amd64/Context.asm index a7165983cf4fa1..a2dbbda0b30991 100644 --- a/src/coreclr/vm/amd64/Context.asm +++ b/src/coreclr/vm/amd64/Context.asm @@ -63,8 +63,9 @@ NESTED_ENTRY ClrRestoreNonvolatileContextWorker, _TEXT mov eax, [r10 + OFFSETOF__CONTEXT__EFlags] push rax popfq + mov rax, [r10 + OFFSETOF__CONTEXT__Rip] mov rsp, [r10 + OFFSETOF__CONTEXT__Rsp] - jmp qword ptr [r10 + OFFSETOF__CONTEXT__Rip] + jmp rax Done_Restore_CONTEXT_CONTROL: ; The function was not asked to restore the control registers so we return back to the caller