Re: [RFC v2 3/7] arm64: cpufeature: handle conflicts based on capability

[Kees Cook <keescook@chromium.org>]

On Wed, May 29, 2019 at 08:03:28PM +0100, Kristina Martsenko wrote:
> Each system capability can be of either boot, local, or system scope,
> depending on when the state of the capability is finalized. When we
> detect a conflict on a late CPU, we either offline the CPU or panic the
> system. We currently always panic if the conflict is caused by a boot
> scope capability, and offline the CPU if the conflict is caused by a
> local or system scope capability.
> 
> We're going to want to add new capability (for pointer authentication)
> which needs to be boot scope but doesn't need to panic the system when a
> conflict is detected. So add a new flag to specify whether the
> capability requires the system to panic or not. Current boot scope
> capabilities are updated to set the flag, so there should be no
> functional change as a result of this patch.
> 
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> 
> Changes since RFC v1:
>  - New patch, to have ptrauth mismatches disable secondaries instead of
>    panicking
> 
>  arch/arm64/include/asm/cpufeature.h | 15 ++++++++++++++-
>  arch/arm64/kernel/cpufeature.c      | 23 +++++++++--------------
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 0522ea674253..ad952f2e0a2b 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -217,6 +217,10 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>   *     In some non-typical cases either both (a) and (b), or neither,
>   *     should be permitted. This can be described by including neither
>   *     or both flags in the capability's type field.
> + *
> + *     In case of a conflict, the CPU is prevented from booting. If the
> + *     ARM64_CPUCAP_PANIC_ON_CONFLICT flag is specified for the capability,
> + *     then a kernel panic is triggered.
>   */
>  
>  
> @@ -249,6 +253,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>  #define ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU	((u16)BIT(4))
>  /* Is it safe for a late CPU to miss this capability when system has it */
>  #define ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU	((u16)BIT(5))
> +/* Panic when a conflict is detected */
> +#define ARM64_CPUCAP_PANIC_ON_CONFLICT		((u16)BIT(6))
>  
>  /*
>   * CPU errata workarounds that need to be enabled at boot time if one or
> @@ -290,7 +296,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>   * CPU feature used early in the boot based on the boot CPU. All secondary
>   * CPUs must match the state of the capability as detected by the boot CPU.
>   */
> -#define ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE ARM64_CPUCAP_SCOPE_BOOT_CPU
> +#define ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE		\
> +	(ARM64_CPUCAP_SCOPE_BOOT_CPU | ARM64_CPUCAP_PANIC_ON_CONFLICT)
>  
>  struct arm64_cpu_capabilities {
>  	const char *desc;
> @@ -354,6 +361,12 @@ cpucap_late_cpu_permitted(const struct arm64_cpu_capabilities *cap)
>  	return !!(cap->type & ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU);
>  }
>  
> +static inline bool
> +cpucap_panic_on_conflict(const struct arm64_cpu_capabilities *cap)
> +{
> +	return !!(cap->type & ARM64_CPUCAP_PANIC_ON_CONFLICT);
> +}
> +
>  /*
>   * Generic helper for handling capabilties with multiple (match,enable) pairs
>   * of call backs, sharing the same capability bit.
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 166584deaed2..8a595b4cb0aa 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1796,10 +1796,8 @@ static void __init enable_cpu_capabilities(u16 scope_mask)
>   * Run through the list of capabilities to check for conflicts.
>   * If the system has already detected a capability, take necessary
>   * action on this CPU.
> - *
> - * Returns "false" on conflicts.
>   */
> -static bool verify_local_cpu_caps(u16 scope_mask)
> +static void verify_local_cpu_caps(u16 scope_mask)
>  {
>  	int i;
>  	bool cpu_has_cap, system_has_cap;
> @@ -1844,10 +1842,12 @@ static bool verify_local_cpu_caps(u16 scope_mask)
>  		pr_crit("CPU%d: Detected conflict for capability %d (%s), System: %d, CPU: %d\n",
>  			smp_processor_id(), caps->capability,
>  			caps->desc, system_has_cap, cpu_has_cap);
> -		return false;
> -	}
>  
> -	return true;
> +		if (cpucap_panic_on_conflict(caps))
> +			cpu_panic_kernel();
> +		else
> +			cpu_die_early();
> +	}
>  }
>  
>  /*
> @@ -1857,12 +1857,8 @@ static bool verify_local_cpu_caps(u16 scope_mask)
>  static void check_early_cpu_features(void)
>  {
>  	verify_cpu_asid_bits();
> -	/*
> -	 * Early features are used by the kernel already. If there
> -	 * is a conflict, we cannot proceed further.
> -	 */
> -	if (!verify_local_cpu_caps(SCOPE_BOOT_CPU))
> -		cpu_panic_kernel();
> +
> +	verify_local_cpu_caps(SCOPE_BOOT_CPU);
>  }
>  
>  static void
> @@ -1910,8 +1906,7 @@ static void verify_local_cpu_capabilities(void)
>  	 * check_early_cpu_features(), as they need to be verified
>  	 * on all secondary CPUs.
>  	 */
> -	if (!verify_local_cpu_caps(SCOPE_ALL & ~SCOPE_BOOT_CPU))
> -		cpu_die_early();
> +	verify_local_cpu_caps(SCOPE_ALL & ~SCOPE_BOOT_CPU);
>  
>  	verify_local_elf_hwcaps(arm64_elf_hwcaps);
>  
> -- 
> 2.11.0
> 

-- 
Kees Cook

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