[PATCH 0/3] arm64 sigreturn unwinding fixes

[PATCH 0/3] arm64 sigreturn unwinding fixes

[Will Deacon]

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

[PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[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.
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

[PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn

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

[PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Mark Brown]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Dave P Martin]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Dave Martin]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Dave Martin]

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

Re: [PATCH 2/3] arm64: vdso: Add a comment to justify the mysterious NOP in sigreturn

[Dave Martin]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Will Deacon]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Will Deacon]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Dave Martin]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Mark Brown]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Dave Martin]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Will Deacon]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Will Deacon]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Daniel Kiss]

> 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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Mark Brown]

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

Re: [PATCH 3/3] arm64: vdso: Fix CFI directives in sigreturn trampoline

[Will Deacon]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Dave Martin]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Mark Brown]

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

Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction

[Dave Martin]

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