Fix nonvolatile context restoration by janvorli · Pull Request #101709 · dotnet/runtime

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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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 · 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

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

+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
typedefstructDECLSPEC_ALIGN(8) _CONTEXT {
//
// Control flags.
//
DWORD ContextFlags;
//
// Integer registers
//
DWORDR0;
DWORDR1;
DWORDR2;
DWORDR3;
DWORDR4;
DWORDR5;
DWORDR6;
DWORDR7;
DWORDR8;
DWORDR9;
DWORDR10;
DWORDR11;
DWORDR12;
//
// 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

@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

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-analysisBot 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:mainApr 30, 2024

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-actionsBot locked and limited conversation to collaborators Jun 4, 2024