Re: [PATCH 05/11] arm64: enable ptrauth earlier

From: James Morse <james.morse@arm•com>
To: Amit Daniel Kachhap <amit.kachhap@arm•com>,
	linux-arm-kernel@lists•infradead.org
Cc: Mark Rutland <mark.rutland@arm•com>,
	Kees Cook <keescook@chromium•org>,
	Suzuki K Poulose <suzuki.poulose@arm•com>,
	Catalin Marinas <catalin.marinas@arm•com>,
	Ard Biesheuvel <ard.biesheuvel@linaro•org>,
	Will Deacon <will.deacon@arm•com>,
	Kristina Martsenko <kristina.martsenko@arm•com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm•com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm•com>,
	Dave Martin <Dave.Martin@arm•com>
Subject: Re: [PATCH 05/11] arm64: enable ptrauth earlier
Date: Wed, 23 Oct 2019 18:32:57 +0100	[thread overview]
Message-ID: <dc7ea4ea-5113-8420-22e4-56ed49cc0da9@arm.com> (raw)
In-Reply-To: <1571300065-10236-6-git-send-email-amit.kachhap@arm.com>

Hi Amit,

On 17/10/2019 09:14, Amit Daniel Kachhap wrote:
> From: Kristina Martsenko <kristina.martsenko@arm•com>
> 
> When the kernel is compiled with pointer auth instructions, the boot CPU
> needs to start using address auth very early, so change the cpucap to
> account for this.
> 
> Pointer auth must be enabled before we call C functions, because it is
> not possible to enter a function with pointer auth disabled and exit it
> with pointer auth enabled. Note, mismatches between architected and
> IMPDEF algorithms will still be caught by the cpufeature framework (the
> separate *_ARCH and *_IMP_DEF cpucaps).
> 
> Note the change in behavior: if the boot CPU has address auth and a late
> CPU does not, then we offline the late CPU. Until now we would have just
> disabled address auth in this case.
> 
> Leave generic authentication as a "system scope" cpucap for now, since
> initially the kernel will only use address authentication.

> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index e58e5975..157c811 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -13,6 +13,7 @@
>  #include <linux/init.h>
>  #include <linux/irqchip/arm-gic-v3.h>
>  
> +#include <asm/alternative.h>
>  #include <asm/assembler.h>
>  #include <asm/boot.h>
>  #include <asm/ptrace.h>
> @@ -119,6 +120,8 @@ ENTRY(stext)
>  	 * the TCR will have been set.
>  	 */
>  	bl	__cpu_setup			// initialise processor
> +	mov	x1, #1
> +	bl	__ptrauth_setup
>  	b	__primary_switch
>  ENDPROC(stext)
>  
> @@ -713,6 +716,8 @@ secondary_startup:
>  	 */
>  	bl	__cpu_secondary_check52bitva
>  	bl	__cpu_setup			// initialise processor
> +	mov	x1, #0
> +	bl	__ptrauth_setup
>  	adrp	x1, swapper_pg_dir
>  	bl	__enable_mmu
>  	ldr	x8, =__secondary_switched

__cpu_setup creates the SCTLR_EL1 value for us, it already reads ID registers for stuff
like AFDBM. It seems odd that you don't do the ptrauth check in there.

Not putting it in __cpu_setup means you've missed the other caller: sleep.S's cpu_resume,
which brings the wakeup CPU back as if it were a secondary. (although the value set at
boot will be restored in _cpu_resume).

It looks like you only need this to be separate to pass in the primary/secondary flag, as
__ptrauth_setup has to work with 3 cases: the boot-CPU and secondary CPUs that must have
the feature, or can ignore the feature. Three cases with one alternative isn't possible.

Could we pull the '__cpu_secondary_checkptrauth' out, and run it earlier? This means the
setup call doesn't need to consider secondary CPUs that don't support ptrauth. (and it
matches what we do for 52bit support)

I think passing primary-boot-cpu into __cpu_setup is something we are going to need for
other features, so it makes sense to add it as a preparatory patch.

Now the setup call can enable the feature if its supported and we are the boot cpu.
If the feature is discovered, cpufeature can change that code to enable it unconditionally
as we know secondaries without support will be caught in __cpu_secondary_checkptrauth.

I think this would be simpler, but the proof is in the writing... what do you think?

Thanks,

James

> @@ -832,6 +837,49 @@ __no_granule_support:
>  	early_park_cpu
>  ENDPROC(__no_granule_support)
>  
> +/*
> + * Enable pointer authentication.
> + *   x0 = SCTLR_EL1
> + *   x1 = 1 for primary, 0 for secondary
> + */
> +__ptrauth_setup:
> +#ifdef CONFIG_ARM64_PTR_AUTH
> +	/* Check if the CPU supports ptrauth */
> +	mrs	x2, id_aa64isar1_el1
> +	ubfx	x2, x2, #ID_AA64ISAR1_APA_SHIFT, #8
> +	cbz	x2, 2f
> +
> +	/* x2 = system_supports_address_auth() */
> +alternative_if ARM64_HAS_ADDRESS_AUTH
> +	mov	x2, 1
> +alternative_else
> +	mov	x2, 0
> +alternative_endif
> +	orr	x2, x2, x1	// primary || system_supports_address_auth()
> +	cbz	x2, 3f
> +
> +	/* Enable ptrauth instructions */
> +	ldr	x2, =SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | \
> +		     SCTLR_ELx_ENDA | SCTLR_ELx_ENDB
> +	orr	x0, x0, x2
> +	b	3f
> +
> +2:	/* No ptrauth support */
> +alternative_if ARM64_HAS_ADDRESS_AUTH
> +	b	4f
> +alternative_else_nop_endif
> +3:
> +#endif
> +	ret
> +
> +#ifdef CONFIG_ARM64_PTR_AUTH
> +4:	/* Park the secondary CPU */
> +	update_early_cpu_boot_status \
> +		CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_NO_PTRAUTH, x0, x1
> +	early_park_cpu
> +#endif
> +ENDPROC(__ptrauth_setup)
> +
>  #ifdef CONFIG_RELOCATABLE
>  __relocate_kernel:
>  	/*

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