Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix nonvolatile context restoration #101709

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Fix nonvolatile context restoration #101709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b212ff
Fix nonvolatile context restoration
janvorli Apr 30, 2024
04726bc
Fix arm and x86
janvorli Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/coreclr/pal/src/arch/arm/context2.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Comment on lines +165 to +167
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility that these stores accidentally overwrite some of the registers in the context that we are going to use later, if the context happens to be at this location in the stack? Or does the context not end with the GPR registers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context contains GPR registers first, then all the NEON ones, debug registers and two DWORDS of padding at the end. So worst case scenario, if we ever overwritten anything from the context, it could only be the padding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here:

runtime/src/coreclr/pal/inc/pal.h

Lines 1739 to 1797 in 017593d

typedef struct DECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORD R0;
DWORD R1;
DWORD R2;
DWORD R3;
DWORD R4;
DWORD R5;
DWORD R6;
DWORD R7;
DWORD R8;
DWORD R9;
DWORD R10;
DWORD R11;
DWORD R12;
//
// Control Registers
//
DWORD Sp;
DWORD Lr;
DWORD Pc;
DWORD Cpsr;
//
// Floating Point/NEON Registers
//
DWORD Fpscr;
DWORD Padding;
union {
NEON128 Q[16];
ULONGLONG D[32];
DWORD S[32];
};
//
// Debug registers
//
DWORD Bvr[ARM_MAX_BREAKPOINTS];
DWORD Bcr[ARM_MAX_BREAKPOINTS];
DWORD Wvr[ARM_MAX_WATCHPOINTS];
DWORD Wcr[ARM_MAX_WATCHPOINTS];
DWORD Padding2[2];
} CONTEXT, *PCONTEXT, *LPCONTEXT;

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)]
Expand Down
17 changes: 12 additions & 5 deletions src/coreclr/pal/src/arch/i386/context2.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/amd64/Context.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/vm/amd64/Context.asm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading