| Message ID | 1594368010-4419-3-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: add Armv8.3 pointer authentication enhancements | expand |
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@arm.com> > [Suzuki: Introduce new matching function for address authentication] > Suggested-by: Suzuki K Poulose <suzuki.poulose@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. 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? I may be missing something here. > + return false; > +} > + > +static bool has_address_auth_metacap(const struct arm64_cpu_capabilities *entry, > + int scope) > +{ > + return has_address_auth_cpucap(cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH_ARCH], scope) || > + has_address_auth_cpucap(cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH_IMP_DEF], scope); > } > > static bool has_generic_auth(const struct arm64_cpu_capabilities *entry, > @@ -1958,7 +1996,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .sign = FTR_UNSIGNED, > .field_pos = ID_AA64ISAR1_APA_SHIFT, > .min_field_value = ID_AA64ISAR1_APA_ARCHITECTED, > - .matches = has_cpuid_feature, > + .matches = has_address_auth_cpucap, > }, > { > .desc = "Address authentication (IMP DEF algorithm)", > @@ -1968,12 +2006,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .sign = FTR_UNSIGNED, > .field_pos = ID_AA64ISAR1_API_SHIFT, > .min_field_value = ID_AA64ISAR1_API_IMP_DEF, > - .matches = has_cpuid_feature, > + .matches = has_address_auth_cpucap, > }, > { > .capability = ARM64_HAS_ADDRESS_AUTH, > .type = ARM64_CPUCAP_BOOT_CPU_FEATURE, > - .matches = has_address_auth, > + .matches = has_address_auth_metacap, > }, > { > .desc = "Generic authentication (architected algorithm)", [...] Cheers ---Dave
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@arm.com> >> [Suzuki: Introduce new matching function for address authentication] >> Suggested-by: Suzuki K Poulose <suzuki.poulose@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
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@arm.com> > >>[Suzuki: Introduce new matching function for address authentication] > >>Suggested-by: Suzuki K Poulose <suzuki.poulose@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. > >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? > >>+ 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. I think the framework could be improved to handle this kind of thing more gracefully, but I don't see an obviously better way to do this for now, especially if this is just a one-off case. (I certainly hope we don't get more features that behave this way...) Cheers ---Dave
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@arm.com> >>>> [Suzuki: Introduce new matching function for address authentication] >>>> Suggested-by: Suzuki K Poulose <suzuki.poulose@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
Hi, On 7/29/20 10:39 PM, Suzuki K Poulose wrote: > 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@arm.com> >>>>> [Suzuki: Introduce new matching function for address authentication] >>>>> Suggested-by: Suzuki K Poulose <suzuki.poulose@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. I just scanned for dsb in the boot flow. Is this dsb(ishst) called from update_cpu_boot_status() in __cpu_up(arch/arm64/kernel/smp.c) sufficient to sync information with the booting cpus? > > >> >>>> 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. This is clean method. I will use it in my next v5 version. Thanks, Amit D > > Suzuki
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; + + /* 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); + } + } + return false; +} + +static bool has_address_auth_metacap(const struct arm64_cpu_capabilities *entry, + int scope) +{ + return has_address_auth_cpucap(cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH_ARCH], scope) || + has_address_auth_cpucap(cpu_hwcaps_ptrs[ARM64_HAS_ADDRESS_AUTH_IMP_DEF], scope); } static bool has_generic_auth(const struct arm64_cpu_capabilities *entry, @@ -1958,7 +1996,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .sign = FTR_UNSIGNED, .field_pos = ID_AA64ISAR1_APA_SHIFT, .min_field_value = ID_AA64ISAR1_APA_ARCHITECTED, - .matches = has_cpuid_feature, + .matches = has_address_auth_cpucap, }, { .desc = "Address authentication (IMP DEF algorithm)", @@ -1968,12 +2006,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .sign = FTR_UNSIGNED, .field_pos = ID_AA64ISAR1_API_SHIFT, .min_field_value = ID_AA64ISAR1_API_IMP_DEF, - .matches = has_cpuid_feature, + .matches = has_address_auth_cpucap, }, { .capability = ARM64_HAS_ADDRESS_AUTH, .type = ARM64_CPUCAP_BOOT_CPU_FEATURE, - .matches = has_address_auth, + .matches = has_address_auth_metacap, }, { .desc = "Generic authentication (architected algorithm)",
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@arm.com> [Suzuki: Introduce new matching function for address authentication] Suggested-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- Changes since v3: * Commit logs cleanup. arch/arm64/kernel/cpufeature.c | 56 ++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 9 deletions(-)