Re: [BUG][PATCH] arm64: bti: fix BTI to handle local indirect branches

[Jeremy Linton]

Hi,


On 10/5/20 2:59 PM, Mark Brown wrote:
> On Mon, Oct 05, 2020 at 01:18:04PM -0500, Jeremy Linton wrote:
>
> > The AES code uses a 'br x7' as part of a function called by
> > a macro, that ends up needing a BTI_J as a target. Lets
> > define SYN_CODE_START_LOCAL() for this and replace the
> > SYM_FUNC_START_LOCAL with a SYM_FUNC_CODE_LOCAL in the AES block.
>
> Really what the subject here should say is that this code is not a
> standard function and therefore should not be annotated as such - it's
> wrong with or without BTI, BTI just makes it very apparent.  It'd also
> be better to split the change in linkage.h out into a separate patch,
> that'd make things clearer for review.
>
> >    CPU: 1 PID: 265 Comm: cryptomgr_test Not tainted 5.8.11-300.fc33.aarch64 #1
> >    pstate: 20400c05 (nzCv daif +PAN -UAO BTYPE=j-)
> >    pc : aesbs_encrypt8+0x0/0x5f0 [aes_neon_bs]
> >    lr : aesbs_xts_encrypt+0x48/0xe0 [aes_neon_bs]
> >    sp : ffff80001052b730
> >    x29: ffff80001052b730 x28: 0000000000000001
> >    x27: ffff0001ec8f4000 x26: ffff0001ec5d27b0
>
> Please think hard before including complete backtraces in upstream
> reports, they are very large and contain almost no useful information
> relative to their size so often obscure the relevant content in your
> message. If part of the backtrace is usefully illustrative (it often is
> for search engines if nothing else) then it's usually better to pull out
> the relevant sections.
>
> > -SYM_FUNC_START_LOCAL(aesbs_encrypt8)
> > +SYM_CODE_START_LOCAL(aesbs_encrypt8)
> >   	ldr		q9, [bskey], #16		// round 0 key
> >   	ldr		q8, M0SR
> >   	ldr		q24, SR
> > @@ -488,10 +488,10 @@ SYM_FUNC_START_LOCAL(aesbs_encrypt8)
> >   	eor		v2.16b, v2.16b, v12.16b
> >   	eor		v5.16b, v5.16b, v12.16b
> >   	ret
> > -SYM_FUNC_END(aesbs_encrypt8)
> > +SYM_END(aesbs_encrypt8)
>
> This should be SYM_CODE_END() to match the opening.  However...
>
> >    * When using in-kernel BTI we need to ensure that PCS-conformant assembly
> > @@ -42,6 +43,9 @@
> >   	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
> >   	BTI_C
> >   
> > +#define SYM_CODE_START_LOCAL(name)			\
> > +	SYM_START(name, SYM_L_LOCAL, SYM_A_ALIGN)       \
> > +	BTI_JC
>
> ...this is going to cause problems, SYM_CODE means that we should
> assemble *exactly* what was written since it's some non-standard thing -
> we use it for the vectors table for example.  Looking at the code it's
> not 100% clear that the best approach here isn't just to change the call
> to a regular function call, this isn't a fast path or anything as far as
> I can see so it's unclear to me why we need to tail call.

Well for some workloads its could be AFAIK. OTOH, Ard mentioned dumping 
the tail call too, and I think that is pretty reasonable. So it looks 
like that is a better plan since it also avoids all this SYM_ flailing.


> Failing that I think we need an annotation for tail called functions,
> that'd need to be a new thing as I am not seeing anything appropriate in
> the current generic annotations.