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

From: Dave Martin <Dave.Martin@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Tamas Zsoldos <tamas.zsoldos@arm.com>,
	kernel-team@android.com, Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Daniel Kiss <daniel.kiss@arm.com>
Subject: Re: [PATCH 1/3] arm64: vdso: Don't prefix sigreturn trampoline with a BTI C instruction
Date: Tue, 19 May 2020 14:25:38 +0100	[thread overview]
Message-ID: <20200519132538.GE5031@arm.com> (raw)
In-Reply-To: <20200519123843.GJ4611@sirena.org.uk>

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