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

From: Dave Martin <Dave.Martin@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>,
	Mark Brown <broonie@kernel.org>,
	kernel-team@android.com, linux-arm-kernel@lists.infradead.org,
	Daniel Kiss <daniel.kiss@arm.com>
Subject: Re: [PATCH v2 1/2] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Date: Wed, 20 May 2020 10:33:55 +0100	[thread overview]
Message-ID: <20200520093354.GJ5031@arm.com> (raw)
In-Reply-To: <20200519162821.16857-2-will@kernel.org>

On Tue, May 19, 2020 at 05:28:20PM +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.
> Commit c91db232da48 ("arm64: vdso: Convert to modern assembler annotations")
> 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,
> and do the same for the 32-bit VDSO as well for good measure.
> 
> 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: c91db232da48 ("arm64: vdso: Convert to modern assembler annotations")
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/vdso/sigreturn.S   | 11 +++++++++--
>  arch/arm64/kernel/vdso32/sigreturn.S | 16 ++++++++--------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
> index 3fb13b81f780..0c921130002a 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.
> + */

Can we cross-reference or duplicate (perhaps abridged) this comment for
vdso32?

Can we also fix the comment by the definition of SYM_FUNC_START()?

SYM_FUNC_START() supersedes ENTRY only for PCS-conformant function entry
points.  Any code with a wacky special-case interface should not not be
using this.

[...]

> +SYM_CODE_START(__kernel_rt_sigreturn)
>  	.cfi_startproc
>  	.cfi_signal_frame
>  	.cfi_def_cfa	x29, 0
> @@ -24,6 +31,6 @@ SYM_FUNC_START(__kernel_rt_sigreturn)
>  	mov	x8, #__NR_rt_sigreturn
>  	svc	#0
>  	.cfi_endproc
> -SYM_FUNC_END(__kernel_rt_sigreturn)
> +SYM_CODE_END(__kernel_rt_sigreturn)
>  
>  emit_aarch64_feature_1_and
> diff --git a/arch/arm64/kernel/vdso32/sigreturn.S b/arch/arm64/kernel/vdso32/sigreturn.S
> index 620524969696..b36d4e2267a3 100644
> --- a/arch/arm64/kernel/vdso32/sigreturn.S
> +++ b/arch/arm64/kernel/vdso32/sigreturn.S
> @@ -17,39 +17,39 @@
>  	.save {r0-r15}
>  	.pad #COMPAT_SIGFRAME_REGS_OFFSET
>  	nop
> -SYM_FUNC_START(__kernel_sigreturn_arm)
> +SYM_CODE_START(__kernel_sigreturn_arm)

...although do we actually need this?  32-bit doesn't have BTI.

But for the reasons given above, this is not a "function" and so
SYM_FUNC_START() is trap for future maintenance even if it makes no
difference now.

[...]

Either way,

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel