arm64 sigreturn unwinding fixes
Will Deacon
maintainercover arm64 sigreturn unwinding fixes
Hi folks,
Here are a handful of sigreturn unwinding fixes, based on top of for-next/bti. Note that I haven't confirmed the GDB breakage, I only spotted it by reading the code.
Daniel, Tamas: please can you confirm that these fix your unwinding issues with LLVM?
Given that this has always been broken and there's a risk of introducing a new regression, I plan to queue these for 5.8 so that we can revert bits if necessary.
Thanks,
Will
--->8
Will Deacon (3):
arm64: vdso: Don't prefix sigreturn trampoline with a BTI C
instruction
arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn
arm64: vdso: Fix CFI directives in sigreturn trampoline
arch/arm64/kernel/vdso/sigreturn.S | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerPATCH 1/3 arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
For better or worse, GDB relies on the exact instruction sequence in the VDSO sigreturn trampoline in order to unwind from signals correctly. Commit 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI") unfortunately added a BTI C instruction to the start of __kernel_rt_sigreturn, which breaks this check. Thankfully, it's also not required, since the trampoline is called from a RET instruction when returning from the signal handler
Remove the unnecessary BTI C instruction from __kernel_rt_sigreturn.
--- arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm64/kernel/vdso/sigreturn.S +8 -1
@@ -15,7 +15,14 @@ .text nop -SYM_FUNC_START(__kernel_rt_sigreturn) +/* + * GDB relies on being able to identify the sigreturn instruction sequence to + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully, + * this function is only ever called from a RET and so omitting the landing pad + * is perfectly fine. + */ +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) .cfi_startproc .cfi_signal_frame .cfi_def_cfa x29, 0
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 1 line · Will Deacon
Fixes 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")
I'd say it's the annotation conversion not this, and also that the bikeshed would be most fetching in orange.
c91db232da484851 (arm64: vdso: Convert to modern assembler annotations)
arch/arm64/kernel/vdso/sigreturn.S line 25
Quoted 9 lines
arch/arm64/kernel/vdso/sigreturn.S +8
-1-SYM_FUNC_START(__kernel_rt_sigreturn) +/* + * GDB relies on being able to identify the sigreturn instruction sequence to + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully, + * this function is only ever called from a RET and so omitting the landing pad + * is perfectly fine. + */ +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)
Shouldn't this be a SYM_CODE_START()? It's the same thing as above currently and we'll break an awful lot more if we change what it does in a way that affects the code, plus the use of CODE basically says the above - it's a "this is non-standard and we know exactly what we're doing, don't mess with it" annotation. If not then it'd be good to cover that in the comment since otherwise this seems like it's asking for a cleanup, we shouldn't really have raw SYM_START() in code.
I guess we also ought to annotate the 32 bit sigreturns as CODE too, though it's academic there without BTI.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 27 lines · Mark Brown
Quoted 1 line · Will Deacon
Fixes 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")I'd say it's the annotation conversion not this, and also that the bikeshed would be most fetching in orange.
c91db232da484851(arm64: vdso: Convert to modern assembler annotations)arch/arm64/kernel/vdso/sigreturn.S line 25
Quoted 9 lines
arch/arm64/kernel/vdso/sigreturn.S +8
-1-SYM_FUNC_START(__kernel_rt_sigreturn) +/* + * GDB relies on being able to identify the sigreturn instruction sequence to + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully, + * this function is only ever called from a RET and so omitting the landing pad + * is perfectly fine. + */ +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)Shouldn't this be a
SYM_CODE_START()? It's the same thing as above currently and we'll break an awful lot more if we change what it does in a way that affects the code, plus the use of CODE basically says the above - it's a "this is non-standard and we know exactly what we're doing, don't mess with it" annotation. If not then it'd be good to cover that in the comment since otherwise this seems like it's asking for a cleanup, we shouldn't really have rawSYM_START()in code.
Quoted 2 lines
I guess we also ought to annotate the 32 bit sigreturns as CODE too, though it's academic there without BTI.
Relating to this, we explicitly don't support calls to __kernel_rt_sigreturn.
Rather, the "ret lr" that jumps here is supposed to be authenticated via pointer auth in the caller.
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 2 lines · Dave Martin
Rather, the "ret lr" that jumps here is supposed to be authenticated via pointer auth in the caller.
In which case there was an issue anyway...
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
I can't remember anything that distinguishes it from an explicit NOP.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 6 lines · Mark Brown
Quoted 2 lines · Dave Martin
Rather, the "ret lr" that jumps here is supposed to be authenticated via pointer auth in the caller.
In which case there was an issue anyway...
What issue?
Quoted 6 lines
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
I can't remember anything that distinguishes it from an explicit NOP.
I think it rejects everything other then fallthrough execution (BTYPE==0, which includes RET). I might have misunderstood something somewhere, but sort of feels like the right thing here. I never put a lot of effort into trying to understand BTI {nothing} though. It seemed a weird, possibly useless special case.
Of course, if gdb's unwinder does rely on recognising magic instruction sequences in the sigreturn trampoline even when the vdso has valid unwind information, we're probably doomed to stick for the current code forever.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 9 lines · Dave Martin
Quoted 6 lines · Mark Brown
Quoted 2 lines · Dave Martin
Rather, the "ret lr" that jumps here is supposed to be authenticated via pointer auth in the caller.
In which case there was an issue anyway...
What issue?
None, I was confused.
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
I can't remember anything that distinguishes it from an explicit NOP.
I think it rejects everything other then fallthrough execution (BTYPE==0, which includes RET). I might have misunderstood something
Right, but since BTI only generates an exception when BTYPE != 0 I'm having trouble differentiating this from a NOP in practical terms.
Quoted 3 lines
somewhere, but sort of feels like the right thing here. I never put a lot of effort into trying to understand BTI {nothing} though. It seemed a weird, possibly useless special case.
That was my read too.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 18 lines · Mark Brown
Quoted 9 lines · Dave Martin
Quoted 6 lines · Mark Brown
Quoted 2 lines · Dave Martin
Rather, the "ret lr" that jumps here is supposed to be authenticated via pointer auth in the caller.
In which case there was an issue anyway...
What issue?
None, I was confused.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
I can't remember anything that distinguishes it from an explicit NOP.
I think it rejects everything other then fallthrough execution (BTYPE==0, which includes RET). I might have misunderstood something
Right, but since BTI only generates an exception when BTYPE != 0 I'm having trouble differentiating this from a NOP in practical terms.
The idea would be that if an attacker could fudge some function pointer to point at __kernel_rt_sigreturn, attempting to do a call via that pointer would still trigger a BTI trap.
This vulnerability isn't applicable to return addresses, because the victim is supposed to sign those before storing them to (attackable) memory, and authenticate between loading back from memory and doing the RET. So the victim can defend itself from that scenario.
Quoted 6 lines
Quoted 4 lines
somewhere, but sort of feels like the right thing here. I never put a lot of effort into trying to understand BTI {nothing} though. It seemed a weird, possibly useless special case.
That was my read too.
And if the gdb doesn't tolerate modification of the exact insn sequence, we can't do it anyway. I'd really say that's a bug-like rogue heuristic in gdb and "not our problem". But people will moan about regressions nonetheless.
I was that interested because of the potential use for BTI {nothing}. I'd have to actually try it out to be 100% sure it works anyway.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 13 lines · Dave Martin
Quoted 11 lines · Mark Brown
Quoted 9 lines · Dave Martin
Quoted 6 lines
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
I can't remember anything that distinguishes it from an explicit NOP.
I think it rejects everything other then fallthrough execution (BTYPE==0, which includes RET). I might have misunderstood something
Quoted 6 lines
Quoted 2 lines
Right, but since BTI only generates an exception when BTYPE != 0 I'm having trouble differentiating this from a NOP in practical terms.
The idea would be that if an attacker could fudge some function pointer to point at
__kernel_rt_sigreturn, attempting to do a call via that pointer would still trigger a BTI trap.
We'll get a BTI exception no matter what instruction is here so long as it's not an appropriate BTI landing pad so unless we want to prevent one being generated there's no need to change the instruction sequence. Or perhaps I'm not quite getting the scenario you're thinking of?
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 17 lines · Mark Brown
Quoted 15 lines · Dave Martin
Quoted 14 lines · Mark Brown
Quoted 9 lines · Dave Martin
Quoted 6 lines
Quoted 4 lines
If BTI {nothing} allows this while disallowing all BR/BLR then we could use that (I can't remember what BTI {nothing} is useful for, if anything).
Otherwise, it's less clear what we should have here.
I can't remember anything that distinguishes it from an explicit NOP.
I think it rejects everything other then fallthrough execution (BTYPE==0, which includes RET). I might have misunderstood something
Right, but since BTI only generates an exception when BTYPE != 0 I'm having trouble differentiating this from a NOP in practical terms.
Quoted 8 lines
Quoted 3 lines
The idea would be that if an attacker could fudge some function pointer to point at
__kernel_rt_sigreturn, attempting to do a call via that pointer would still trigger a BTI trap.We'll get a BTI exception no matter what instruction is here so long as it's not an appropriate BTI landing pad so unless we want to prevent one being generated there's no need to change the instruction sequence. Or perhaps I'm not quite getting the scenario you're thinking of?
Duh, yes. I guess we're good, then.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerRe: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
In reply to Mark BrownQuoted 8 lines · Mark Brown
Quoted 1 line · Will Deacon
Fixes 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")I'd say it's the annotation conversion not this, and also that the bikeshed would be most fetching in orange.
c91db232da484851(arm64: vdso: Convert to modern assembler annotations)
I initially had both, but that felt weird so I dropped this one. However, I'm happy to switch it for v2.
Quoted 18 lines
arch/arm64/kernel/vdso/sigreturn.S line 25
Quoted 9 lines
arch/arm64/kernel/vdso/sigreturn.S +8
-1-SYM_FUNC_START(__kernel_rt_sigreturn) +/* + * GDB relies on being able to identify the sigreturn instruction sequence to + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully, + * this function is only ever called from a RET and so omitting the landing pad + * is perfectly fine. + */ +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN)Shouldn't this be a
SYM_CODE_START()? It's the same thing as above currently and we'll break an awful lot more if we change what it does in a way that affects the code, plus the use of CODE basically says the above - it's a "this is non-standard and we know exactly what we're doing, don't mess with it" annotation. If not then it'd be good to cover that in the comment since otherwise this seems like it's asking for a cleanup, we shouldn't really have rawSYM_START()in code.
Quoted 2 lines
I guess we also ought to annotate the 32 bit sigreturns as CODE too, though it's academic there without BTI.
Yes, that's much better, I'll use SYM_CODE_START() and update the compat code too (I'd forgotten it wasn't an array of hex anymore).
Thanks,
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
In reply to Will DeaconQuoted 2 lines · Will Deacon
For better or worse, GDB relies on the exact instruction sequence in the VDSO sigreturn trampoline in order to unwind from signals correctly.
Are you sure? I'm struggling to find the relevant code in gdb.
Quoted 46 lines
Commit
714a8d02ca4d("arm64: asm: OverrideSYM_FUNC_STARTwhen building the kernel with BTI") unfortunately added a BTI C instruction to the start of__kernel_rt_sigreturn, which breaks this check. Thankfully, it's also not required, since the trampoline is called from a RET instruction when returning from the signal handlerRemove the unnecessary BTI C instruction from
__kernel_rt_sigreturn.Cc Dave Martin <[email protected]>Cc Mark Brown <[email protected]>Cc Daniel Kiss <[email protected]>Cc Tamas Zsoldos <[email protected]>Fixes 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)arch/arm64/kernel/vdso/sigreturn.S +8
-1@@ -15,7 +15,14 @@ .text nop -SYM_FUNC_START(__kernel_rt_sigreturn) +/* + * GDB relies on being able to identify the sigreturn instruction sequence to + * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() + * here, as it will emit a BTI C instruction and break the unwinder. Thankfully, + * this function is only ever called from a RET and so omitting the landing pad + * is perfectly fine. + */ +SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) .cfi_startproc .cfi_signal_frame .cfi_def_cfa x29, 0Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerRe: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Quoted 5 lines · Dave Martin
Quoted 2 lines · Will Deacon
For better or worse, GDB relies on the exact instruction sequence in the VDSO sigreturn trampoline in order to unwind from signals correctly.
Are you sure? I'm struggling to find the relevant code in gdb.
It looks pretty damning:
(and also look at struct tramp_frame).
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerPATCH 2/3 arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn
In reply to Will DeaconEvery so often we have to remind ourselves about the purpose of the weird NOP instruction immediately preceding the sigreturn trampoline.
Add a short comment to state that it exists for some unwinders that determine the caller address by subtracting from the return address.
--- arch/arm64/kernel/vdso/sigreturn.S | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/kernel/vdso/sigreturn.S +6 -1
@@ -14,7 +14,12 @@ .text - nop +/* + * This mysterious NOP is required for some unwinders that subtract one from + * the return address in order to identify the calling function. + * Hack borrowed from arch/powerpc/kernel/vdso64/sigtramp.S. + */ + nop // Mysterious NOP /* * GDB relies on being able to identify the sigreturn instruction sequence to * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn
Quoted 19 lines · Will Deacon
Every so often we have to remind ourselves about the purpose of the weird NOP instruction immediately preceding the sigreturn trampoline.
Add a short comment to state that it exists for some unwinders that determine the caller address by subtracting from the return address.
Cc Dave Martin <[email protected]>Cc Daniel Kiss <[email protected]>Cc Tamas Zsoldos <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/vdso/sigreturn.S | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)arch/arm64/kernel/vdso/sigreturn.S +0
-0
arch/arm64/kernel/vdso/sigreturn.S line 22
Quoted 10 lines
arch/arm64/kernel/vdso/sigreturn.S +6
-1@@ -14,7 +14,12 @@ .text - nop +/* + * This mysterious NOP is required for some unwinders that subtract one from + * the return address in order to identify the calling function. + * Hack borrowed from arch/powerpc/kernel/vdso64/sigtramp.S. + */ + nop // Mysterious NOP
Quoted 11 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-0/* * GDB relies on being able to identify the sigreturn instruction sequence to * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START()Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerPATCH 3/3 arm64: vdso: Fix CFI directives in sigreturn trampoline
In reply to Will DeaconDaniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.
This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
--- arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
arch/arm64/kernel/vdso/sigreturn.S +3 -5
@@ -14,6 +14,9 @@ .text +/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.
@@ -28,11 +31,6 @@ * is perfectly fine. */ SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8 mov x8, #__NR_rt_sigreturn svc #0 .cfi_endproc
Signature · 7 lines
2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Quoted 37 lines · Will Deacon
Daniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc Tamas Zsoldos <[email protected]>Reported-by Dave Martin <[email protected]>Reported-by Daniel Kiss <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)arch/arm64/kernel/vdso/sigreturn.S +3
-0@@ -14,6 +14,9 @@ .text +/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.
arch/arm64/kernel/vdso/sigreturn.S line 35
Quoted 8 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-5@@ -28,11 +31,6 @@ * is perfectly fine. */ SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8
Having thought about this again, I think it might be better to stick to the original version.
If the signal handler is halfway through mungeing the sigcontext then backtracing using sigcontext won't be reliable.
In any case, if something in the interrupted code caused the signal, the backtrace of the old stack is likely to me more useful, and that's what x29 will give us. If there's no old stack because we blew it away, that's too bad.
Plus, in the absence of any spec that says exactly what .cfi_signal_frame means*, we probably don't want to rock the boat.
Cheers
---Dave
[*] assumption, but the arch ABI and Dwarf specs are unlikely to cover this, and Linux doesn't go in for specs.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerRe: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Quoted 50 lines · Dave P Martin
Quoted 37 lines · Will Deacon
Daniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc Tamas Zsoldos <[email protected]>Reported-by Dave Martin <[email protected]>Reported-by Daniel Kiss <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)arch/arm64/kernel/vdso/sigreturn.S +3
-0@@ -14,6 +14,9 @@ .text +/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.arch/arm64/kernel/vdso/sigreturn.S line 35
Quoted 8 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-5@@ -28,11 +31,6 @@ * is perfectly fine. */ SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8Having thought about this again, I think it might be better to stick to the original version.
Quoted 2 lines
If the signal handler is halfway through mungeing the sigcontext then backtracing using sigcontext won't be reliable.
I suppose, but then what does .cfi_signal_frame do? I'll see if I can find something that uses it. The frame record is still sitting on the stack, so it does feel redundant to say both '.cfi_signal_frame' and '.cfi_def_cfa' (and other architectures, e.g. riscv don't do this).
But I'm also happy to play it safe if I can stick a comment in here saying what it does.
Quoted 2 lines
Plus, in the absence of any spec that says exactly what .cfi_signal_frame means*, we probably don't want to rock the boat.
The gas docs say:
"Mark current function as signal trampoline."
which is really informative.
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Quoted 62 lines · Will Deacon
Quoted 50 lines · Dave P Martin
Quoted 37 lines · Will Deacon
Daniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc Tamas Zsoldos <[email protected]>Reported-by Dave Martin <[email protected]>Reported-by Daniel Kiss <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)arch/arm64/kernel/vdso/sigreturn.S +3
-0@@ -14,6 +14,9 @@ .text +/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.arch/arm64/kernel/vdso/sigreturn.S line 35
Quoted 8 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-5@@ -28,11 +31,6 @@ * is perfectly fine. */ SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8Having thought about this again, I think it might be better to stick to the original version.
Quoted 2 lines
If the signal handler is halfway through mungeing the sigcontext then backtracing using sigcontext won't be reliable.
I suppose, but then what does .cfi_signal_frame do? I'll see if I can find something that uses it. The frame record is still sitting on the stack, so it does feel redundant to say both '.cfi_signal_frame' and '.cfi_def_cfa' (and other architectures, e.g. riscv don't do this).
But I'm also happy to play it safe if I can stick a comment in here saying what it does.
Quoted 8 lines
Quoted 2 lines
Plus, in the absence of any spec that says exactly what .cfi_signal_frame means*, we probably don't want to rock the boat.
The gas docs say:
"Mark current function as signal trampoline."
which is really informative.
Well, we've demonstrated that identifying the signal frame is a gross bodge. The cfi annotation should provide a reliable way to identify the signal frame, but I guess it was too poorly specified or came too late to prevent the bodges from spreading.
Since this seems to be a nonstandard invention, I wouldn't hold out much hope of finding a usable spec.
Of course, something might be using it now, so I guess we have to leave it.
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerRe: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Quoted 12 lines · Dave Martin
Quoted 5 lines · Will Deacon
The gas docs say:
"Mark current function as signal trampoline."
which is really informative.
Well, we've demonstrated that identifying the signal frame is a gross bodge. The cfi annotation should provide a reliable way to identify the signal frame, but I guess it was too poorly specified or came too late to prevent the bodges from spreading.
Quoted 5 lines
Since this seems to be a nonstandard invention, I wouldn't hold out much hope of finding a usable spec.
Of course, something might be using it now, so I guess we have to leave it.
I had a quick look at libstdc++ (the horror!) and it looks like the DWARF backend in there /does/ make use of this information as part of the _Unwind_GetIPInfo() function:
*ip_before_insn is set to 1 or 0 depending on whether or not the PC corresponds to a function annotated with .cfi_signal_frame. So I think the code in libstdc++-v3/libsupc++/eh_personality.cc doesn't need the mysterious NOP at all!
Unfortunately, it looks like the LLVM libc++ doesn't use this, and instead calls _Unwind_GetIP():
and unconditionally subtracts 1 in libcxxabi/src/cxa_personality.cpp, meaning that the NOP is necessary.
So, after giving myself a splitting headache, it looks like:
1. We need the mysterious NOP for LLVM
2. We could drop .cfi_signal_frame but it's harmless to keep it
3. We need the .cfi_def_cfa directive to locate the frame record, as .cfi_signal_frame doesn't do very much at all.
Make sense? If so, I'll spin a v2 of the patches along with a comment trying to summarise some of this.
Cheers,
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
In reply to Dave MartinQuoted 37 lines · Dave Martin
Quoted 32 lines · Will Deacon
Quoted 31 lines · Dave P Martin
Quoted 30 lines · Will Deacon
Daniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc Tamas Zsoldos <[email protected]>Reported-by Dave Martin <[email protected]>Reported-by Daniel Kiss <[email protected]>Signed-off-by Will Deacon <[email protected]>---
arch/arm64/kernel/vdso/sigreturn.S| 8 +++-----1 file changed, 3 insertions(+), 5 deletions(-)arch/arm64/kernel/vdso/sigreturn.S +0
-0@@ -14,6 +14,9 @@ .text
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
arch/arm64/kernel/vdso/sigreturn.S +3
-0+/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.@@ -28,11 +31,6 @@Quoted 8 lines
Quoted 8 lines
arch/arm64/kernel/vdso/sigreturn.S line 35
Quoted 8 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-5* is perfectly fine. */ SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8
LLVM’s unwinder does not like this version of the CFI. It needs a bit more information, the cfi_signal_frame is not used for finding the frame.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Having thought about this again, I think it might be better to stick to the original version.
If the signal handler is halfway through mungeing the sigcontext then backtracing using sigcontext won't be reliable.
Quoted 7 lines
Quoted 7 lines
I suppose, but then what does .cfi_signal_frame do? I'll see if I can find something that uses it. The frame record is still sitting on the stack, so it does feel redundant to say both '.cfi_signal_frame' and '.cfi_def_cfa' (and other architectures, e.g. riscv don't do this).
But I'm also happy to play it safe if I can stick a comment in here saying what it does.
Sounds good to me.
Quoted 15 lines
Quoted 9 lines
Quoted 3 lines
Plus, in the absence of any spec that says exactly what .cfi_signal_frame means*, we probably don't want to rock the boat.
The gas docs say:
"Mark current function as signal trampoline."
which is really informative.
Well, we've demonstrated that identifying the signal frame is a gross bodge. The cfi annotation should provide a reliable way to identify the signal frame, but I guess it was too poorly specified or came too late to prevent the bodges from spreading.
Quoted 7 lines
Since this seems to be a nonstandard invention, I wouldn't hold out much hope of finding a usable spec.
Of course, something might be using it now, so I guess we have to leave it.
---Dave
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon
maintainerRe: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline
Quoted 43 lines · Daniel Kiss
Quoted 43 lines · Dave Martin
Quoted 41 lines · Will Deacon
Quoted 40 lines · Dave P Martin
Quoted 39 lines · Will Deacon
Daniel reports that the .cfi_startproc is misplaced for the sigreturn trampoline, which causes LLVM's unwinder to misbehave:
| I run into this with LLVM’s unwinder.
| This combination was always broken.This prompted Dave to realise that our CFI directives are contradictory, as we specify both .cfi_signal_frame *and* .cfi_def_cfa, with the latter unconditionally identifying the interrupted context as opposed to the values in the sigcontext.
Rework the CFI directives so that we only use .cfi_signal_frame, and include the "mysterious NOP" as part of the .cfi_{start,end}proc block.
Cc Tamas Zsoldos <[email protected]>Reported-by Dave Martin <[email protected]>Reported-by Daniel Kiss <[email protected]>Signed-off-by Will Deacon <[email protected]>---
arch/arm64/kernel/vdso/sigreturn.S| 8 +++-----1 file changed, 3 insertions(+), 5 deletions(-)arch/arm64/kernel/vdso/sigreturn.S +3
-0@@ -14,6 +14,9 @@ .text +/* Ensure that the mysterious NOP can be associated with a function. */ + .cfi_startproc + .cfi_signal_frame /* * This mysterious NOP is required for some unwinders that subtract one from * the return address in order to identify the calling function.@@ -28,11 +31,6 @@ * is perfectly fine. */
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
arch/arm64/kernel/vdso/sigreturn.S line 35
Quoted 6 lines
arch/arm64/kernel/vdso/sigreturn.S +0
-5SYM_START(__kernel_rt_sigreturn, SYM_L_GLOBAL, SYM_A_ALIGN) - .cfi_startproc - .cfi_signal_frame - .cfi_def_cfa x29, 0 - .cfi_offset x29, 0 * 8 - .cfi_offset x30, 1 * 8LLVM’s unwinder does not like this version of the CFI. It needs a bit more information, the
cfi_signal_frameis not used for finding the frame.
Thanks, Daniel. That is, at least, aligned with my current understanding of how this is supposed to work.
I'll send out a v2 in a bit.
Will
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel