[PATCH v4 2/4] arm64: cpufeature: Modify address authentication cpufeature to exact

[Suzuki K Poulose]

On 07/29/2020 11:36 AM, Dave Martin wrote:
> On Fri, Jul 10, 2020 at 01:30:08PM +0530, Amit Daniel Kachhap wrote:
>> The current address authentication cpufeature levels are set as LOWER_SAFE
>> which is not compatible with the different configurations added for Armv8.3
>> ptrauth enhancements as the different levels have different behaviour and
>> there is no tunable to enable the lower safe versions. This is rectified
>> by setting those cpufeature type as EXACT.
>>
>> The current cpufeature framework also does not interfere in the booting of
>> non-exact secondary cpus but rather marks them as tainted. As a workaround
>> this is fixed by replacing the generic match handler with a new handler
>> specific to ptrauth.
>>
>> After this change, if there is any variation in ptrauth configurations in
>> secondary cpus from boot cpu then those mismatched cpus are parked in an
>> infinite loop.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap at arm.com>
>> [Suzuki: Introduce new matching function for address authentication]
>> Suggested-by: Suzuki K Poulose <suzuki.poulose at arm.com>
>> ---
>> Changes since v3:
>>   * Commit logs cleanup.
>>
>>   arch/arm64/kernel/cpufeature.c | 56 ++++++++++++++++++++++++++++------
>>   1 file changed, 47 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 9fae0efc80c1..8ac8c18f70c9 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -210,9 +210,9 @@ static const struct arm64_ftr_bits ftr_id_aa64isar1[] = {
>>   	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_FCMA_SHIFT, 4, 0),
>>   	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_JSCVT_SHIFT, 4, 0),
>>   	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> -		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_API_SHIFT, 4, 0),
>> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR1_API_SHIFT, 4, 0),
>>   	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_PTR_AUTH),
>> -		       FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_APA_SHIFT, 4, 0),
>> +		       FTR_STRICT, FTR_EXACT, ID_AA64ISAR1_APA_SHIFT, 4, 0),
>>   	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_DPB_SHIFT, 4, 0),
>>   	ARM64_FTR_END,
>>   };
>> @@ -1605,11 +1605,49 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
>>   #endif /* CONFIG_ARM64_RAS_EXTN */
>>   
>>   #ifdef CONFIG_ARM64_PTR_AUTH
>> -static bool has_address_auth(const struct arm64_cpu_capabilities *entry,
>> -			     int __unused)
>> +static bool has_address_auth_cpucap(const struct arm64_cpu_capabilities *entry, int scope)
>>   {
>> -	return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) ||
>> -	       __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF);
>> +	int local_cpu_auth;
>> +	static int boot_cpu_auth_arch;
>> +	static int boot_cpu_auth_imp_def;
> 
> I guess having static variables here is probably safe, provided that
> calls to this function are strictly serialised with sufficiently
> heavyweight synchronisation.
> 

These are initialised with the primary boot CPU. And later on called
from CPU bring up. So they are serialized, except when
this_cpu_has_cap() could be called. But this is fine, as it is a read
operation.

> Might be worth a comment.
> 
> Coming up with a cleaner approach using locks or atomics might be
> overkill, but other folks might take a stronger view on this.
> 
>> +
>> +	/* We don't expect to be called with SCOPE_SYSTEM */
>> +	WARN_ON(scope == SCOPE_SYSTEM);
>> +
>> +	local_cpu_auth = cpuid_feature_extract_field(__read_sysreg_by_encoding(entry->sys_reg),
>> +						     entry->field_pos, entry->sign);
>> +	/*
>> +	 * The ptr-auth feature levels are not intercompatible with
>> +	 * lower levels. Hence we must match all the CPUs with that
>> +	 * of the boot CPU. So cache the level of boot CPU and compare
>> +	 * it against the secondary CPUs.
>> +	 */
>> +	if (scope & SCOPE_BOOT_CPU) {
>> +		if (entry->capability == ARM64_HAS_ADDRESS_AUTH_IMP_DEF) {
>> +			boot_cpu_auth_imp_def = local_cpu_auth;
>> +			return boot_cpu_auth_imp_def >= entry->min_field_value;
>> +		} else if (entry->capability == ARM64_HAS_ADDRESS_AUTH_ARCH) {
>> +			boot_cpu_auth_arch = local_cpu_auth;
>> +			return boot_cpu_auth_arch >= entry->min_field_value;
>> +		}
>> +	} else if (scope & SCOPE_LOCAL_CPU) {
>> +		if (entry->capability == ARM64_HAS_ADDRESS_AUTH_IMP_DEF) {
>> +			return (local_cpu_auth >= entry->min_field_value) &&
>> +			       (local_cpu_auth == boot_cpu_auth_imp_def);
>> +		}
>> +		if (entry->capability == ARM64_HAS_ADDRESS_AUTH_ARCH) {
>> +			return (local_cpu_auth >= entry->min_field_value) &&
>> +			       (local_cpu_auth == boot_cpu_auth_arch);
>> +		}
>> +	}
> 
> This looks complex.  I guess this is something to do with the change to
> FTR_EXACT, but won't the cpufeatures code do the right thing anyway with
> mismatched features?

Part of the issue is that the capability is SCOPE_BOOT_CPU. So,
we need to verify the features on the secondary CPUs, making sure that
they match the "level" as detected by the boot CPU. Otherwise, we could
end up with mismatched CPUs, where all of them support different levels
of AUTH.

Also, implies that the checks are performed before the cpufeature gets
updated with the secondary CPUs values. Any conflict implies the
secondary CPU ends up parked.

So, the "FTR_EXACT" change only has an impact of keeping the cpufeature
infrastructure sanitised only in the absence of CONFIG_ARM64_PTRAUTH.

Cheers
Suzuki