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

From: Catalin Marinas <catalin.marinas@arm.com>
To: Peter Collingbourne <pcc@google.com>
Cc: Evgenii Stepanov <eugenis@google.com>,
	Kostya Serebryany <kcc@google.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Dave Martin <Dave.Martin@arm.com>, Will Deacon <will@kernel.org>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH v2] arm64: allow TCR_EL1.TBID0 to be configured
Date: Tue, 27 Jul 2021 17:51:09 +0100	[thread overview]
Message-ID: <20210727165109.GB13920@arm.com> (raw)
In-Reply-To: <20210622051204.3682580-1-pcc@google.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

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