* [PATCH 0/3] arm64 sigreturn unwinding fixes @ 2020-05-19 12:18 Will Deacon 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw) To: linux-arm-kernel Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin, Daniel Kiss 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 Cc: Dave Martin <dave.martin@arm.com> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> Cc: Daniel Kiss <daniel.kiss@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: <kernel-team@android.com> --->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(-) -- 2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 12:18 [PATCH 0/3] arm64 sigreturn unwinding fixes Will Deacon @ 2020-05-19 12:18 ` Will Deacon 2020-05-19 12:38 ` Mark Brown 2020-05-19 13:21 ` Dave Martin 2020-05-19 12:18 ` [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn Will Deacon 2020-05-19 12:18 ` [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 2 siblings, 2 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw) To: linux-arm-kernel Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin, Daniel Kiss 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. Cc: Dave Martin <dave.martin@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: Daniel Kiss <daniel.kiss@arm.com> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI") Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S index 3fb13b81f780..83ac284dae79 100644 --- a/arch/arm64/kernel/vdso/sigreturn.S +++ b/arch/arm64/kernel/vdso/sigreturn.S @@ -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 -- 2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon @ 2020-05-19 12:38 ` Mark Brown 2020-05-19 13:25 ` Dave Martin 2020-05-19 15:26 ` Will Deacon 2020-05-19 13:21 ` Dave Martin 1 sibling, 2 replies; 22+ messages in thread From: Mark Brown @ 2020-05-19 12:38 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, kernel-team, Dave Martin, linux-arm-kernel, Daniel Kiss [-- Attachment #1.1: Type: text/plain, Size: 1403 bytes --] On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote: > 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) > -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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 12:38 ` Mark Brown @ 2020-05-19 13:25 ` Dave Martin 2020-05-19 14:35 ` Mark Brown 2020-05-19 15:26 ` Will Deacon 1 sibling, 1 reply; 22+ messages in thread From: Dave Martin @ 2020-05-19 13:25 UTC (permalink / raw) To: Mark Brown Cc: Tamas Zsoldos, kernel-team, Will Deacon, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 01:38:43PM +0100, Mark Brown wrote: > On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote: > > > 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) > > > -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. 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 13:25 ` Dave Martin @ 2020-05-19 14:35 ` Mark Brown 2020-05-19 14:55 ` Dave Martin 0 siblings, 1 reply; 22+ messages in thread From: Mark Brown @ 2020-05-19 14:35 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, kernel-team, Will Deacon, linux-arm-kernel, Daniel Kiss [-- Attachment #1.1: Type: text/plain, Size: 490 bytes --] On Tue, May 19, 2020 at 02:25:38PM +0100, Dave Martin wrote: > 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... > 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 14:35 ` Mark Brown @ 2020-05-19 14:55 ` Dave Martin 2020-05-19 15:42 ` Mark Brown 0 siblings, 1 reply; 22+ messages in thread From: Dave Martin @ 2020-05-19 14:55 UTC (permalink / raw) To: Mark Brown Cc: Tamas Zsoldos, Will Deacon, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 03:35:00PM +0100, Mark Brown wrote: > On Tue, May 19, 2020 at 02:25:38PM +0100, Dave Martin wrote: > > > 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? > > 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 14:55 ` Dave Martin @ 2020-05-19 15:42 ` Mark Brown 2020-05-20 9:48 ` Dave Martin 0 siblings, 1 reply; 22+ messages in thread From: Mark Brown @ 2020-05-19 15:42 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Will Deacon, kernel-team, linux-arm-kernel, Daniel Kiss [-- Attachment #1.1: Type: text/plain, Size: 1172 bytes --] On Tue, May 19, 2020 at 03:55:15PM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 03:35:00PM +0100, Mark Brown wrote: > > On Tue, May 19, 2020 at 02:25:38PM +0100, Dave Martin wrote: > > > 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. > > > 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. > 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 15:42 ` Mark Brown @ 2020-05-20 9:48 ` Dave Martin 2020-05-20 10:46 ` Mark Brown 0 siblings, 1 reply; 22+ messages in thread From: Dave Martin @ 2020-05-20 9:48 UTC (permalink / raw) To: Mark Brown Cc: Tamas Zsoldos, kernel-team, Will Deacon, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 04:42:47PM +0100, Mark Brown wrote: > On Tue, May 19, 2020 at 03:55:15PM +0100, Dave Martin wrote: > > On Tue, May 19, 2020 at 03:35:00PM +0100, Mark Brown wrote: > > > On Tue, May 19, 2020 at 02:25:38PM +0100, Dave Martin wrote: > > > > > 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. > > > > > 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. 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. > > > 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-20 9:48 ` Dave Martin @ 2020-05-20 10:46 ` Mark Brown 2020-05-20 11:08 ` Dave Martin 0 siblings, 1 reply; 22+ messages in thread From: Mark Brown @ 2020-05-20 10:46 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, kernel-team, Will Deacon, linux-arm-kernel, Daniel Kiss [-- Attachment #1.1: Type: text/plain, Size: 1258 bytes --] On Wed, May 20, 2020 at 10:48:45AM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 04:42:47PM +0100, Mark Brown wrote: > > On Tue, May 19, 2020 at 03:55:15PM +0100, Dave Martin wrote: > > > > > 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. > 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-20 10:46 ` Mark Brown @ 2020-05-20 11:08 ` Dave Martin 0 siblings, 0 replies; 22+ messages in thread From: Dave Martin @ 2020-05-20 11:08 UTC (permalink / raw) To: Mark Brown Cc: Tamas Zsoldos, Will Deacon, kernel-team, linux-arm-kernel, Daniel Kiss On Wed, May 20, 2020 at 11:46:53AM +0100, Mark Brown wrote: > On Wed, May 20, 2020 at 10:48:45AM +0100, Dave Martin wrote: > > On Tue, May 19, 2020 at 04:42:47PM +0100, Mark Brown wrote: > > > On Tue, May 19, 2020 at 03:55:15PM +0100, Dave Martin wrote: > > > > > > > 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. > > > 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 12:38 ` Mark Brown 2020-05-19 13:25 ` Dave Martin @ 2020-05-19 15:26 ` Will Deacon 1 sibling, 0 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 15:26 UTC (permalink / raw) To: Mark Brown Cc: Tamas Zsoldos, kernel-team, Dave Martin, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 01:38:43PM +0100, Mark Brown wrote: > On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote: > > > 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. > > -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. 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon 2020-05-19 12:38 ` Mark Brown @ 2020-05-19 13:21 ` Dave Martin 2020-05-19 13:29 ` Will Deacon 1 sibling, 1 reply; 22+ messages in thread From: Dave Martin @ 2020-05-19 13:21 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote: > 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. > 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. > > Cc: Dave Martin <dave.martin@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Daniel Kiss <daniel.kiss@arm.com> > Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> > Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI") > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/vdso/sigreturn.S | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > index 3fb13b81f780..83ac284dae79 100644 > --- a/arch/arm64/kernel/vdso/sigreturn.S > +++ b/arch/arm64/kernel/vdso/sigreturn.S > @@ -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 > -- > 2.26.2.761.g0e0b3e54be-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction 2020-05-19 13:21 ` Dave Martin @ 2020-05-19 13:29 ` Will Deacon 0 siblings, 0 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 13:29 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 02:21:01PM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 01:18:16PM +0100, Will Deacon wrote: > > 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: https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/aarch64-linux-tdep.c;h=34ba0d87baaff12f1f9711e777ab15a0a394f59b;hb=HEAD#l361 (and also look at struct tramp_frame). Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn 2020-05-19 12:18 [PATCH 0/3] arm64 sigreturn unwinding fixes Will Deacon 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon @ 2020-05-19 12:18 ` Will Deacon 2020-05-19 13:26 ` Dave Martin 2020-05-19 12:18 ` [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 2 siblings, 1 reply; 22+ messages in thread From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw) To: linux-arm-kernel Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin, Daniel Kiss 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 <dave.martin@arm.com> Cc: Daniel Kiss <daniel.kiss@arm.com> Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/vdso/sigreturn.S | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S index 83ac284dae79..7853fa9692f6 100644 --- a/arch/arm64/kernel/vdso/sigreturn.S +++ b/arch/arm64/kernel/vdso/sigreturn.S @@ -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() -- 2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn 2020-05-19 12:18 ` [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn Will Deacon @ 2020-05-19 13:26 ` Dave Martin 0 siblings, 0 replies; 22+ messages in thread From: Dave Martin @ 2020-05-19 13:26 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 01:18:17PM +0100, Will Deacon wrote: > 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 <dave.martin@arm.com> > Cc: Daniel Kiss <daniel.kiss@arm.com> > Cc: Tamas Zsoldos <tamas.zsoldos@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/vdso/sigreturn.S | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > index 83ac284dae79..7853fa9692f6 100644 > --- a/arch/arm64/kernel/vdso/sigreturn.S > +++ b/arch/arm64/kernel/vdso/sigreturn.S > @@ -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 Reviewed-by: Dave Martin <Dave.Martin@arm.com> > /* > * GDB relies on being able to identify the sigreturn instruction sequence to > * unwind from signal handlers. We cannot, therefore, use SYM_FUNC_START() > -- > 2.26.2.761.g0e0b3e54be-goog > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 12:18 [PATCH 0/3] arm64 sigreturn unwinding fixes Will Deacon 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon 2020-05-19 12:18 ` [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn Will Deacon @ 2020-05-19 12:18 ` Will Deacon 2020-05-19 13:09 ` Dave P Martin 2 siblings, 1 reply; 22+ messages in thread From: Will Deacon @ 2020-05-19 12:18 UTC (permalink / raw) To: linux-arm-kernel Cc: Will Deacon, Tamas Zsoldos, Mark Brown, kernel-team, Dave Martin, Daniel Kiss 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 <tamas.zsoldos@arm.com> Reported-by: Dave Martin <dave.martin@arm.com> Reported-by: Daniel Kiss <daniel.kiss@arm.com> Signed-off-by: Will Deacon <will@kernel.org> --- arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S index 7853fa9692f6..28b33f7d0604 100644 --- a/arch/arm64/kernel/vdso/sigreturn.S +++ b/arch/arm64/kernel/vdso/sigreturn.S @@ -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 -- 2.26.2.761.g0e0b3e54be-goog _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 12:18 ` [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon @ 2020-05-19 13:09 ` Dave P Martin 2020-05-19 13:39 ` Will Deacon 0 siblings, 1 reply; 22+ messages in thread From: Dave P Martin @ 2020-05-19 13:09 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote: > 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 <tamas.zsoldos@arm.com> > Reported-by: Dave Martin <dave.martin@arm.com> > Reported-by: Daniel Kiss <daniel.kiss@arm.com> > Signed-off-by: Will Deacon <will@kernel.org> > --- > arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > index 7853fa9692f6..28b33f7d0604 100644 > --- a/arch/arm64/kernel/vdso/sigreturn.S > +++ b/arch/arm64/kernel/vdso/sigreturn.S > @@ -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 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 13:09 ` Dave P Martin @ 2020-05-19 13:39 ` Will Deacon 2020-05-19 13:55 ` Dave Martin 0 siblings, 1 reply; 22+ messages in thread From: Will Deacon @ 2020-05-19 13:39 UTC (permalink / raw) To: Dave P Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 02:09:31PM +0100, Dave P Martin wrote: > On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote: > > 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 <tamas.zsoldos@arm.com> > > Reported-by: Dave Martin <dave.martin@arm.com> > > Reported-by: Daniel Kiss <daniel.kiss@arm.com> > > Signed-off-by: Will Deacon <will@kernel.org> > > --- > > arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > > index 7853fa9692f6..28b33f7d0604 100644 > > --- a/arch/arm64/kernel/vdso/sigreturn.S > > +++ b/arch/arm64/kernel/vdso/sigreturn.S > > @@ -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 > > 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. 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. > 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 13:39 ` Will Deacon @ 2020-05-19 13:55 ` Dave Martin 2020-05-19 15:24 ` Will Deacon 2020-05-19 15:30 ` Daniel Kiss 0 siblings, 2 replies; 22+ messages in thread From: Dave Martin @ 2020-05-19 13:55 UTC (permalink / raw) To: Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 02:39:41PM +0100, Will Deacon wrote: > On Tue, May 19, 2020 at 02:09:31PM +0100, Dave P Martin wrote: > > On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote: > > > 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 <tamas.zsoldos@arm.com> > > > Reported-by: Dave Martin <dave.martin@arm.com> > > > Reported-by: Daniel Kiss <daniel.kiss@arm.com> > > > Signed-off-by: Will Deacon <will@kernel.org> > > > --- > > > arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- > > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > > > index 7853fa9692f6..28b33f7d0604 100644 > > > --- a/arch/arm64/kernel/vdso/sigreturn.S > > > +++ b/arch/arm64/kernel/vdso/sigreturn.S > > > @@ -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 > > > > 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. > > 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. > > > 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 13:55 ` Dave Martin @ 2020-05-19 15:24 ` Will Deacon 2020-05-19 15:30 ` Daniel Kiss 1 sibling, 0 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 15:24 UTC (permalink / raw) To: Dave Martin Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel, Daniel Kiss On Tue, May 19, 2020 at 02:55:37PM +0100, Dave Martin wrote: > On Tue, May 19, 2020 at 02:39:41PM +0100, Will Deacon wrote: > > 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. 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: https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/baselib--unwind-getipinfo.html *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(): https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/baselib--unwind-getip.html 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 13:55 ` Dave Martin 2020-05-19 15:24 ` Will Deacon @ 2020-05-19 15:30 ` Daniel Kiss 2020-05-19 15:55 ` Will Deacon 1 sibling, 1 reply; 22+ messages in thread From: Daniel Kiss @ 2020-05-19 15:30 UTC (permalink / raw) To: Dave P Martin, Will Deacon Cc: Tamas Zsoldos, Mark Brown, kernel-team, linux-arm-kernel > On 19 May 2020, at 15:55, Dave Martin <Dave.Martin@arm.com> wrote: > > On Tue, May 19, 2020 at 02:39:41PM +0100, Will Deacon wrote: >> On Tue, May 19, 2020 at 02:09:31PM +0100, Dave P Martin wrote: >>> On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote: >>>> 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 <tamas.zsoldos@arm.com> >>>> Reported-by: Dave Martin <dave.martin@arm.com> >>>> Reported-by: Daniel Kiss <daniel.kiss@arm.com> >>>> Signed-off-by: Will Deacon <will@kernel.org> >>>> --- >>>> arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- >>>> 1 file changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S >>>> index 7853fa9692f6..28b33f7d0604 100644 >>>> --- a/arch/arm64/kernel/vdso/sigreturn.S >>>> +++ b/arch/arm64/kernel/vdso/sigreturn.S >>>> @@ -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 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. >>> >>> 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. >> >> 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. >> >>> 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline 2020-05-19 15:30 ` Daniel Kiss @ 2020-05-19 15:55 ` Will Deacon 0 siblings, 0 replies; 22+ messages in thread From: Will Deacon @ 2020-05-19 15:55 UTC (permalink / raw) To: Daniel Kiss Cc: Tamas Zsoldos, Mark Brown, kernel-team, Dave P Martin, linux-arm-kernel On Tue, May 19, 2020 at 03:30:57PM +0000, Daniel Kiss wrote: > > On 19 May 2020, at 15:55, Dave Martin <Dave.Martin@arm.com> wrote: > > On Tue, May 19, 2020 at 02:39:41PM +0100, Will Deacon wrote: > >> On Tue, May 19, 2020 at 02:09:31PM +0100, Dave P Martin wrote: > >>> On Tue, May 19, 2020 at 01:18:18PM +0100, Will Deacon wrote: > >>>> 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 <tamas.zsoldos@arm.com> > >>>> Reported-by: Dave Martin <dave.martin@arm.com> > >>>> Reported-by: Daniel Kiss <daniel.kiss@arm.com> > >>>> Signed-off-by: Will Deacon <will@kernel.org> > >>>> --- > >>>> arch/arm64/kernel/vdso/sigreturn.S | 8 +++----- > >>>> 1 file changed, 3 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S > >>>> index 7853fa9692f6..28b33f7d0604 100644 > >>>> --- a/arch/arm64/kernel/vdso/sigreturn.S > >>>> +++ b/arch/arm64/kernel/vdso/sigreturn.S > >>>> @@ -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 > 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. 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 linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-05-20 11:08 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-19 12:18 [PATCH 0/3] arm64 sigreturn unwinding fixes Will Deacon 2020-05-19 12:18 ` [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction Will Deacon 2020-05-19 12:38 ` Mark Brown 2020-05-19 13:25 ` Dave Martin 2020-05-19 14:35 ` Mark Brown 2020-05-19 14:55 ` Dave Martin 2020-05-19 15:42 ` Mark Brown 2020-05-20 9:48 ` Dave Martin 2020-05-20 10:46 ` Mark Brown 2020-05-20 11:08 ` Dave Martin 2020-05-19 15:26 ` Will Deacon 2020-05-19 13:21 ` Dave Martin 2020-05-19 13:29 ` Will Deacon 2020-05-19 12:18 ` [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn Will Deacon 2020-05-19 13:26 ` Dave Martin 2020-05-19 12:18 ` [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline Will Deacon 2020-05-19 13:09 ` Dave P Martin 2020-05-19 13:39 ` Will Deacon 2020-05-19 13:55 ` Dave Martin 2020-05-19 15:24 ` Will Deacon 2020-05-19 15:30 ` Daniel Kiss 2020-05-19 15:55 ` Will Deacon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.