Fix nonvolatile context restoration #101709

janvorli · 2024-04-30T07:04:16Z

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.

Close #101060, #98292

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.

dotnet-policy-service · 2024-04-30T07:04:43Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

jakobbotsch

LGTM! This seems like the most plausible explanation.
Is only x64 affected?

janvorli · 2024-04-30T09:28:51Z

Actually, the other targets use RtlRestoreContext in PAL and it also has this issue on some targets (arm, x86, most likely riscv64 and loongarch64, not sure about s390x and ppc64le). Arm64 is ok. Let me add fixes for more architectures here.

AndyAyersMS · 2024-04-30T20:41:30Z

We expect this to fix #98292 and #101060, right?

Seems like a very small race window, which jives with the fact that these are very hard to repro.

janvorli · 2024-04-30T21:10:14Z

We expect this to fix #98292 and #101060, right?

I think so as they both seem to be OSR related.

janvorli · 2024-04-30T21:17:05Z

@t-mustafin, @shushanhf, @vikasgupta8, @uweigand can you please check if the architectures you have added support for also need a fix like this? It seems to me that RISCV64 and LoongArch64 do need to be fixed, I have no idea about the S390x and PPC64LE.

janvorli · 2024-04-30T21:36:42Z

@jakobbotsch can you please take a look again? I have added arm and x86 fixes. Arm64 was already ok. I will leave possible fixes for the other architectures on the external people who have added support for those.

janvorli · 2024-04-30T21:37:24Z

The CI failure is a known issue.

jakobbotsch · 2024-04-30T22:05:34Z

src/coreclr/pal/src/arch/arm/context2.S

+    str r2, [r1, #-4]
+    ldr r2, [r0, #(CONTEXT_R12)]
+    str r2, [r1, #-8]


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?

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.

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;

jakobbotsch

It looks good to me, just one hypothetical wondering.

t-mustafin · 2024-05-02T12:28:58Z

@t-mustafin, @shushanhf, @vikasgupta8, @uweigand can you please check if the architectures you have added support for also need a fix like this? It seems to me that RISCV64 and LoongArch64 do need to be fixed, I have no idea about the S390x and PPC64LE.

@janvorli thanks for notification. Are this changes needed on release/8.0 branch?

cc @dotnet/samsung

Fix s390x context restoration along the lines of dotnet#101709

uweigand · 2024-05-03T16:42:39Z

@t-mustafin, @shushanhf, @vikasgupta8, @uweigand can you please check if the architectures you have added support for also need a fix like this? It seems to me that RISCV64 and LoongArch64 do need to be fixed, I have no idea about the S390x and PPC64LE.

Thanks for the heads-up! The s390x patch is here: #101854

Fix s390x context restoration along the lines of #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

Fix s390x context restoration along the lines of dotnet#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

Fix s390x context restoration along the lines of dotnet#101709

janvorli added the area-VM-coreclr label Apr 30, 2024

janvorli added this to the 9.0.0 milestone Apr 30, 2024

janvorli requested a review from jakobbotsch April 30, 2024 07:04

janvorli self-assigned this Apr 30, 2024

jakobbotsch approved these changes Apr 30, 2024

View reviewed changes

jakobbotsch linked an issue Apr 30, 2024 that may be closed by this pull request

Test failure: readytorun/coreroot_determinism/coreroot_determinism/coreroot_determinism.cmd #101060

Closed

jakobbotsch mentioned this pull request Apr 30, 2024

[libraries-pgo] segfault in System.Linq.Tests #98292

Closed

jakobbotsch linked an issue Apr 30, 2024 that may be closed by this pull request

[libraries-pgo] segfault in System.Linq.Tests #98292

Closed

build-analysis bot mentioned this pull request Apr 30, 2024

System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanScalarDestination_SpecialValues fails #101721

Closed

Fix arm and x86

04726bc

jakobbotsch reviewed Apr 30, 2024

View reviewed changes

jakobbotsch approved these changes Apr 30, 2024

View reviewed changes

janvorli merged commit 7aefd27 into dotnet:main Apr 30, 2024
87 of 89 checks passed

janvorli deleted the fix-nonvolatile-context-restoration branch April 30, 2024 22:47

uweigand added a commit to uweigand/runtime that referenced this pull request May 3, 2024

s390x: Fix nonvolatile context restoration

40b6428

Fix s390x context restoration along the lines of dotnet#101709

uweigand mentioned this pull request May 3, 2024

s390x: Fix nonvolatile context restoration #101854

Merged

AndyAyersMS mentioned this pull request May 3, 2024

Test failure: System.Collections.Concurrent.Tests #92574

Closed

t-mustafin mentioned this pull request May 3, 2024

[RISC-V] Fix context restoration as #101709 describes #101865

Merged

shushanhf mentioned this pull request May 6, 2024

[LoongArch64] Fix nonvolatile context restoration referenced 101709. #101904

Merged

janvorli pushed a commit that referenced this pull request May 6, 2024

s390x: Fix nonvolatile context restoration (#101854)

420147a

Fix s390x context restoration along the lines of #101709

michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024

s390x: Fix nonvolatile context restoration (dotnet#101854)

a1792cf

Fix s390x context restoration along the lines of dotnet#101709

janvorli mentioned this pull request May 16, 2024

Update llvm-libunwind to v18 #102231

Merged

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024

s390x: Fix nonvolatile context restoration (dotnet#101854)

b4a2f92

Fix s390x context restoration along the lines of dotnet#101709

github-actions bot locked and limited conversation to collaborators Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nonvolatile context restoration #101709

Fix nonvolatile context restoration #101709

janvorli commented Apr 30, 2024 •

edited

Loading

dotnet-policy-service bot commented Apr 30, 2024

jakobbotsch left a comment

janvorli commented Apr 30, 2024

AndyAyersMS commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

jakobbotsch Apr 30, 2024

janvorli Apr 30, 2024

janvorli Apr 30, 2024

jakobbotsch left a comment

t-mustafin commented May 2, 2024

uweigand commented May 3, 2024

	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;

Fix nonvolatile context restoration #101709

Fix nonvolatile context restoration #101709

Conversation

janvorli commented Apr 30, 2024 • edited Loading

dotnet-policy-service bot commented Apr 30, 2024

jakobbotsch left a comment

Choose a reason for hiding this comment

janvorli commented Apr 30, 2024

AndyAyersMS commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

janvorli commented Apr 30, 2024

jakobbotsch Apr 30, 2024

Choose a reason for hiding this comment

janvorli Apr 30, 2024

Choose a reason for hiding this comment

janvorli Apr 30, 2024

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

t-mustafin commented May 2, 2024

uweigand commented May 3, 2024

janvorli commented Apr 30, 2024 •

edited

Loading