| Message ID | 1586842314-19527-2-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: add Armv8.6 pointer authentication | expand |
On Tue, Apr 14, 2020 at 11:01:51AM +0530, Amit Daniel Kachhap wrote: > This fixes the earlier commit 3ff047f6971d3c ("arm64: cpufeature: Fix > meta-capability cpufeature check"). This patch was added to fix the > dependency of individual meta-cpucaps checks on the array entry order. This > dependency was specifically added for cpufeature of system scope. > > However this dependency can occur for cpufeature of boot scope such as > ARM64_HAS_ADDRESS_AUTH so this patch renames the helper function > __system_matches_cap to __cpufeature_matches_cap and invokes the match > handler with the scope fetched from the cpufeatures array list. > > Fixes: 3ff047f6971d3c ("arm64: cpufeature: Fix meta-capability cpufeature check") > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> Does this patch need to be merged in 5.7? The fixed commit went in 5.7-rc1 but it doesn't look to me like we'd have a problem without this fix. Basically we read the sanitised regs with SYSTEM_SCOPE rather than the current CPU regs. These are already populated correctly to the register values of the boot CPU. Otherwise I'm fine with the patch, just disputing the Fixes tag.
On Wed, May 06, 2020 at 04:00:01PM +0100, Catalin Marinas wrote: > On Tue, Apr 14, 2020 at 11:01:51AM +0530, Amit Daniel Kachhap wrote: > > This fixes the earlier commit 3ff047f6971d3c ("arm64: cpufeature: Fix > > meta-capability cpufeature check"). This patch was added to fix the > > dependency of individual meta-cpucaps checks on the array entry order. This > > dependency was specifically added for cpufeature of system scope. > > > > However this dependency can occur for cpufeature of boot scope such as > > ARM64_HAS_ADDRESS_AUTH so this patch renames the helper function > > __system_matches_cap to __cpufeature_matches_cap and invokes the match > > handler with the scope fetched from the cpufeatures array list. > > > > Fixes: 3ff047f6971d3c ("arm64: cpufeature: Fix meta-capability cpufeature check") > > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > > Does this patch need to be merged in 5.7? The fixed commit went in > 5.7-rc1 but it doesn't look to me like we'd have a problem without this > fix. Basically we read the sanitised regs with SYSTEM_SCOPE rather than Yes, this fixes an actual issue. The code is fine for BOOT cpu when we detect whether the system supports the capability. However, for verifying the secondary CPUs, this still succeeds as we only check the sanitised values and a defective CPU could escape from being parked. I think something like the following is a better idea, to make sure we do the appropriate checks. diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 9fac745aa7bb..5df74490d7d3 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -1373,15 +1373,23 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused) static bool has_address_auth(const struct arm64_cpu_capabilities *entry, int __unused) { - return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || - __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); + if (scope == SCOPE_SYSTEM) + return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || + __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); + else + return this_cpu_has_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || + this_cpu_has_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); } static bool has_generic_auth(const struct arm64_cpu_capabilities *entry, - int __unused) + int scope) { - return __system_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || - __system_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); + if (scope == SCOPE_SYSTEM) + return __system_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || + __system_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); + else + return this_cpu_has_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || + this_cpu_has_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); } #endif /* CONFIG_ARM64_PTR_AUTH */
Hi, On 5/6/20 9:44 PM, Suzuki K Poulose wrote: > On Wed, May 06, 2020 at 04:00:01PM +0100, Catalin Marinas wrote: >> On Tue, Apr 14, 2020 at 11:01:51AM +0530, Amit Daniel Kachhap wrote: >>> This fixes the earlier commit 3ff047f6971d3c ("arm64: cpufeature: Fix >>> meta-capability cpufeature check"). This patch was added to fix the >>> dependency of individual meta-cpucaps checks on the array entry order. This >>> dependency was specifically added for cpufeature of system scope. >>> >>> However this dependency can occur for cpufeature of boot scope such as >>> ARM64_HAS_ADDRESS_AUTH so this patch renames the helper function >>> __system_matches_cap to __cpufeature_matches_cap and invokes the match >>> handler with the scope fetched from the cpufeatures array list. >>> >>> Fixes: 3ff047f6971d3c ("arm64: cpufeature: Fix meta-capability cpufeature check") >>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> >> Does this patch need to be merged in 5.7? The fixed commit went in >> 5.7-rc1 but it doesn't look to me like we'd have a problem without this >> fix. Basically we read the sanitised regs with SYSTEM_SCOPE rather than > > Yes, this fixes an actual issue. The code is fine for BOOT cpu when we > detect whether the system supports the capability. However, for verifying > the secondary CPUs, this still succeeds as we only check the sanitised > values and a defective CPU could escape from being parked. > > I think something like the following is a better idea, to make sure we > do the appropriate checks. This approach looks better. I will use it in the next iteration. Cheers, Amit > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 9fac745aa7bb..5df74490d7d3 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1373,15 +1373,23 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused) > static bool has_address_auth(const struct arm64_cpu_capabilities *entry, > int __unused) > { > - return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || > - __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); > + if (scope == SCOPE_SYSTEM) > + return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || > + __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); > + else > + return this_cpu_has_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || > + this_cpu_has_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); > } > > static bool has_generic_auth(const struct arm64_cpu_capabilities *entry, > - int __unused) > + int scope) > { > - return __system_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || > - __system_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); > + if (scope == SCOPE_SYSTEM) > + return __system_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || > + __system_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); > + else > + return this_cpu_has_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || > + this_cpu_has_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); > } > #endif /* CONFIG_ARM64_PTR_AUTH */ > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 9fac745aa7bb..08795025409c 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -116,7 +116,7 @@ cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); -static bool __system_matches_cap(unsigned int n); +static bool __cpufeature_matches_cap(unsigned int n); /* * NOTE: Any changes to the visibility of features should be kept in @@ -1373,15 +1373,15 @@ static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused) static bool has_address_auth(const struct arm64_cpu_capabilities *entry, int __unused) { - return __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || - __system_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); + return __cpufeature_matches_cap(ARM64_HAS_ADDRESS_AUTH_ARCH) || + __cpufeature_matches_cap(ARM64_HAS_ADDRESS_AUTH_IMP_DEF); } static bool has_generic_auth(const struct arm64_cpu_capabilities *entry, int __unused) { - return __system_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || - __system_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); + return __cpufeature_matches_cap(ARM64_HAS_GENERIC_AUTH_ARCH) || + __cpufeature_matches_cap(ARM64_HAS_GENERIC_AUTH_IMP_DEF); } #endif /* CONFIG_ARM64_PTR_AUTH */ @@ -2251,16 +2251,16 @@ bool this_cpu_has_cap(unsigned int n) /* * This helper function is used in a narrow window when, * - The system wide safe registers are set with all the SMP CPUs and, - * - The SYSTEM_FEATURE cpu_hwcaps may not have been set. + * - The cpu_hwcaps may not have been set. * In all other cases cpus_have_{const_}cap() should be used. */ -static bool __system_matches_cap(unsigned int n) +static bool __cpufeature_matches_cap(unsigned int n) { if (n < ARM64_NCAPS) { const struct arm64_cpu_capabilities *cap = cpu_hwcaps_ptrs[n]; if (cap) - return cap->matches(cap, SCOPE_SYSTEM); + return cap->matches(cap, cpucap_default_scope(cap)); } return false; } @@ -2337,7 +2337,7 @@ void __init setup_cpu_features(void) static bool __maybe_unused cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) { - return (__system_matches_cap(ARM64_HAS_PAN) && !__system_matches_cap(ARM64_HAS_UAO)); + return (__cpufeature_matches_cap(ARM64_HAS_PAN) && !__cpufeature_matches_cap(ARM64_HAS_UAO)); } static void __maybe_unused cpu_enable_cnp(struct arm64_cpu_capabilities const *cap)
This fixes the earlier commit 3ff047f6971d3c ("arm64: cpufeature: Fix meta-capability cpufeature check"). This patch was added to fix the dependency of individual meta-cpucaps checks on the array entry order. This dependency was specifically added for cpufeature of system scope. However this dependency can occur for cpufeature of boot scope such as ARM64_HAS_ADDRESS_AUTH so this patch renames the helper function __system_matches_cap to __cpufeature_matches_cap and invokes the match handler with the scope fetched from the cpufeatures array list. Fixes: 3ff047f6971d3c ("arm64: cpufeature: Fix meta-capability cpufeature check") Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> --- arch/arm64/kernel/cpufeature.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)