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

From: Will Deacon <will@kernel.org>
To: Dave Martin <Dave.Martin@arm.com>
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:53:54 +0100	[thread overview]
Message-ID: <20200520095354.GF24293@willie-the-truck> (raw)
In-Reply-To: <20200520093354.GJ5031@arm.com>

On Wed, May 20, 2020 at 10:33:55AM +0100, Dave Martin wrote:
> 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?

Yes, that's not a bad idea. I'll add a comment to the top of that file.

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

I'll tweak it slightly for v3.

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

Right, it's just done for consistency on the 32-bit side.

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

Thanks!

Will

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