Re: [PATCH v2] arm64: allow TCR_EL1.TBID0 to be configured

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Peter,

On Mon, Jun 21, 2021 at 10:12:04PM -0700, Peter Collingbourne wrote:
> Introduce a command line flag that controls whether TCR_EL1.TBID0
> is set at boot time. Since this is a change to the userspace ABI the
> option defaults to off for now, although it seems likely that we'll
> be able to change the default at some future point.
> 
> Setting TCR_EL1.TBID0 increases the number of signature bits used by
> the pointer authentication instructions for instruction addresses by 8,
> which improves the security of pointer authentication, but it also has
> the consequence of changing the operation of the branch instructions
> so that they no longer ignore the top byte of the target address but
> instead fault if they are non-zero.

I'm a bit uneasy about the ABI change and not so keen on constraining
the ABI through the kernel command line. Ideally we should make this an
opt-in per application (prctl()) but that has some aspects to address
first: (a) this bit is permitted to be cached in the TLB so we'd need
some TLBI when setting it (and a clarification in the specs that it is
tagged by ASID/VMID, probably fine) and (b) we'd need to context-switch
TCR_EL1, with a small performance penalty (I don't think it's
significant but worth testing).

Unfortunately, we can't turn TBID0 off dynamically when we detect a
tagged PC since this would break authentication of already encoded
pointers.

Prior to hwasan and MTE, I doubt anyone would have noticed this change
but once malloc() and friends started returning tagged pointers,
programs executing code from malloc()'ed regions would fall apart with
TBID0. I think it's a bit of stretch to argue that it's hwasan and MTE
causing the application breakage rather than a user-kernel ABI change,
since that's already working currently (though such programs should be
re-written).

Longer term, I'd like the TBID0 to be the default but transitioning
without breaking the user is tricky, hence my first option would be
per-application with an opt-in.

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 871c82ab0a30..9ee32afe121c 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -529,11 +529,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr,
>  	vm_fault_t fault;
>  	unsigned long vm_flags;
>  	unsigned int mm_flags = FAULT_FLAG_DEFAULT;
> -	unsigned long addr = untagged_addr(far);
> +	unsigned long addr;
>  
>  	if (kprobe_page_fault(regs, esr))
>  		return 0;
>  
> +	/*
> +	 * If TBID0 is set then we may get an IABT with a tagged address here as
> +	 * a result of branching to a tagged address. In this case we want to
> +	 * avoid untagging the address, let the VMA lookup fail and get a
> +	 * SIGSEGV. Leaving the address as is will also work if TBID0 is clear
> +	 * or unsupported because the tag bits of FAR_EL1 will be clear.
> +	 */
> +	if (is_el0_instruction_abort(esr))
> +		addr = far;
> +	else
> +		addr = untagged_addr(far);

Should this also check for tcr_tbid0_enabled() before deciding not to
untag the address?

-- 
Catalin