Re: [PATCH 4/8] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, May 24, 2019 at 04:38:48PM +0100, Mark Rutland wrote:
> On Fri, May 24, 2019 at 03:53:06PM +0100, Dave Martin wrote:
> > On Fri, May 24, 2019 at 02:02:17PM +0100, Mark Rutland wrote:
> > > On Fri, May 24, 2019 at 11:25:29AM +0100, Dave Martin wrote:
> > > > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > > > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > > > +{
> > > > +	if (system_supports_bti() && (prot & PROT_BTI_GUARDED))
> > > > +		return VM_ARM64_GP;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > > > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > > > +{
> > > > +	return (vm_flags & VM_ARM64_GP) ? __pgprot(PTE_GP) : __pgprot(0);
> > > > +}
> > > 
> > > While the architectural name for the PTE bit is GP, it might make more
> > > sense to call the vm flag VM_ARM64_BTI, since people are more likely to
> > > recognise BTI than GP as a mnemonic.
> > > 
> > > Not a big deal either way, though.
> > 
> > I'm happy to change it.  It's a kernel internal flag used in
> > approximately zero places.  So whatever name is most intuitive for
> > kernel maintainers is fine.  Nobody else needs to look at it.
> 
> Sure thing; I just know that I'm going to remember what BTI is much more
> easily than I'll remember what GP is.
> 
> > > > diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> > > > index b2de329..b868ef11 100644
> > > > --- a/arch/arm64/include/asm/ptrace.h
> > > > +++ b/arch/arm64/include/asm/ptrace.h
> > > > @@ -41,6 +41,7 @@
> > > >  
> > > >  /* Additional SPSR bits not exposed in the UABI */
> > > >  #define PSR_IL_BIT		(1 << 20)
> > > > +#define PSR_BTYPE_CALL		(2 << 10)
> > > 
> > > I thought BTYPE was a 2-bit field, so isn't there at leat one other
> > > value to have a mnemonic for?
> > > 
> > > Is it an enumeration or a bitmask?
> > 
> > It's a 2-bit enumeration, and for now this is the only value that the
> > kernel uses: this determines the types of BTI landing pad permitted at
> > signal handler entry points in BTI guarded pages.
> > 
> > Possibly it would be clearer to write it
> > 
> > #define PSR_BTYPE_CALL		(0b10 << 10)
> > 
> > but we don't write other ptrace.h constants this way.  In UAPI headers
> > we should avoid GCC-isms, but here it's OK since we already rely on this
> > syntax internally.
> > 
> > I can change it if you prefer, though my preference is to leave it.
> 
> I have no issue with the (2 << 10) form, but could we add mnemonics for
> the other values now, even if we're not using them at this instant?

Can do.  How about.

	PSR_BTYPE_NONE	(0 << 10)
	PSR_BTYPE_JC	(1 << 10)
	PSR_BTYPE_C	(2 << 10)
	PSR_BTYPE_J	(3 << 10)

That matches the way I decode PSTATE for splats.

The architecture does not define mnemonics so these are my invention,
but anyway this is just for the kernel.

> 
> > > >  #endif /* _UAPI__ASM_HWCAP_H */
> > > > diff --git a/arch/arm64/include/uapi/asm/mman.h b/arch/arm64/include/uapi/asm/mman.h
> > > > new file mode 100644
> > > > index 0000000..4776b43
> > > > --- /dev/null
> > > > +++ b/arch/arm64/include/uapi/asm/mman.h
> > > > @@ -0,0 +1,9 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > > +#ifndef _UAPI__ASM_MMAN_H
> > > > +#define _UAPI__ASM_MMAN_H
> > > > +
> > > > +#include <asm-generic/mman.h>
> > > > +
> > > > +#define PROT_BTI_GUARDED	0x10		/* BTI guarded page */
> > > 
> > > From prior discussions, I thought this would be PROT_BTI, without the
> > > _GUARDED suffix. Do we really need that?
> > > 
> > > AFAICT, all other PROT_* definitions only have a single underscore, and
> > > the existing arch-specific flags are PROT_ADI on sparc, and PROT_SAO on
> > > powerpc.
> > 
> > No strong opinon.  I was trying to make the name less obscure, but I'm
> > equally happy with PROT_BTI if people prefer that.
> 
> My personal opinion is that PROT_BTI is preferable, but I'll let others
> chime in.

[...]

If nobody objects, I'll change it as you propose, and change the vma
flag to match.

> > > > @@ -741,6 +741,11 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> > > >  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > >  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > > >  
> > > > +	if (system_supports_bti()) {
> > > > +		regs->pstate &= ~(regs->pstate & PSR_BTYPE_MASK);
> > > 
> > > Nit: that can be:
> > > 
> > > 		regs->pstate &= ~PSR_BTYPE_MASK;
> > 
> > x & ~y is sensitive to the type of y and can clobber high bits, so I
> > prefer not to write it.  GCC generates the same code either way.
> 
> Ah, I thought this might befor type promotion.
> 
> > However, this will also trip us up elsewhere when the time comes, so
> > maybe it's a waste of time working around it here.
> > 
> > If you feel strongly, I'm happy to change it.
> 
> I'd rather we followed the same pattern as elsewhere, as having this
> special case is confusing, and we'd still have the same bug elsewhere.
> 
> My concern here is consistency, so if you want to fix up all instances
> to preserve the upper 32 bits of regs->pstate, I'd be happy. :)
> 
> I also think there are nicer/clearer ways to fix the type promotion
> issue, like using UL in the field definitions, using explicit casts, or
> adding helpers to set/clear bits with appropriate promotion.

Sure, I change it to be more consistent.

Wrapping this idiom up in a clear_bits() wrapper might be an idea, 
but in advance of that it makes little sense to do it just in this one
place.

I don't really like annotating header #defines with arbitrary types
until it's really necessary, so I'll leave it for now.

> > > > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
> > > > index 5610ac0..85b456b 100644
> > > > --- a/arch/arm64/kernel/syscall.c
> > > > +++ b/arch/arm64/kernel/syscall.c
> > > > @@ -66,6 +66,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
> > > >  	unsigned long flags = current_thread_info()->flags;
> > > >  
> > > >  	regs->orig_x0 = regs->regs[0];
> > > > +	regs->pstate &= ~(regs->pstate & PSR_BTYPE_MASK);
> > > 
> > > Likewise:
> > > 
> > > 	regs->pstate &= ~PSR_BTYPE_MASK;
> > > 
> > > ... though I don't understand why that would matter to syscalls, nor how
> > > those bits could ever be set given we had to execute an SVC to get here.
> > > 
> > > What am I missing?
> > 
> > The behaviour is counterintuivite here.  The architecture guarantees to
> > preserve BTYPE for traps, faults and asynchronous exceptions, but for a
> > synchronous execption from normal architectural execution of an
> > exception-generating instruction (SVC/HVC/SMC) the architecture leaves
> > it IMP DEF whether BTYPE is preserved or zeroed in SPSR.
> 
> I'm still missing something here. IIUC were BTYPE was non-zero, we
> should take the BTI trap before executing the SVC/HVC/SMC, right?
> 
> Otherwise, it would be possible to erroneously branch to an SVC/HVC/SMC,
> which would logically violate the BTI protection.

Only if the SVC (etc.) is in a BTI guarded page.  Otherwise, we could
have legitimately branched to the SVC insn directly and BTYPE would
be nonzero, but no trap would occur.

We should still logically zero BTYPE across SVC in that case, because 
the SVC may itself branch:  a signal could be delivered on return and
we want the prevailing BTYPE then to be 0 for capture in the signal
frame.  Doing this zeroing in signal delivery because if the signal
is not delivered in SVE then a nonzero BTYPE might be live, and we
must then restore it properly on sigreturn.

As you observe, this scenario should be impossible if the SVC insn
is in a guarded page, unless userspace does something foolhardy like
catching the SIGILL and fudging BTYPE or the return address.

> If the assumption is that software can fix that case up, and the ??C
> exception is prioritized above the BTI exception, then I think that we
> should check whether it was permitted rather than silently fixing it up.

I don't think there is any silent fixup here: if SVC is executed
immediately after a branch then the BTI exception should preempt the
architectural execution of the SVC: if the SVC is architecturally
executed at all, any preceding branch must have been compliant.

> > I suppose precisely because there's only one way to reach the SVC
> > handler, software knows for certain whether zero SPSR.BTYPE in that
> > case.  So hardware doesn't need to do it.
> 
> As above, I thought BTYPE had to be zero in order for it to be possible
> to execute the SVC/HVC/SMC, but there might be caveats.

Modulo the GP bit in the page table (as described in more detail above).

There may be caveats I've missed though -- I need to take another look.

Feel free to continue digging :)


(Now I come to think of it I also need to look at rseq etc., which is
another magic kernel-mediated branch mechanism.)

Cheers
---Dave