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

[Suzuki K Poulose]

On 07/29/2020 03:31 PM, Dave Martin wrote:
> On Wed, Jul 29, 2020 at 01:28:06PM +0100, Suzuki K Poulose wrote:
>> 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.
> 
> To guarantee that any update to those variables is visible to a booting
> CPU, we'd probably need a DSB and/or DMB somewhere.  For now I don't see
> anything explicity, though it's likely we're getting them as a side
> effect of something else.
> 
> In any case, I guess we've been managing for some time without this
> being strictly correct.
> 

That is a good point. In fact we need this for init_cpu_features() and 
update_cpu_features() too.


> 
>>> 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;
> 
> Actually, can we make use of read_sanitised_ftr_reg() here rather than
> having to cache the values in static variables?

Thats actually a good idea ! However the only catch is if there is a
case where the values become compatible. e.g, APA >= 3 are all
compatible, in which case the sanitised value would indicate 0.
If we decide not support such cases, then we could go ahead with  the
sanitised_ftr value.

Suzuki