From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5BED5C433DF for ; Wed, 29 Jul 2020 14:33:17 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 24786207E8 for ; Wed, 29 Jul 2020 14:33:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FgBl9YLT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 24786207E8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=QyhnXt19SKUM2hEK/VSkBbmmQ/FYZzhNneqzmOFzbMY=; b=FgBl9YLTpNc8Pj3SWqza4IAnh DZSEtDgEWi89BVsxo9U8FN+LykuUIieBHeWf2dEJd/Gnr7D1KB+DDkZwVQuHC4DZAgKMnAMN/1dAa 4uRYPNAcpAShYbQn6BRWUeAEgamu6Rxdyu2/2OBgpgIoJFN+yfiV4fvL0Pa65rTUiMbBP3dxp0Y95 UdymEwjM+XGtejZZlRpcFk0j+byS1VhzCaE4TCm4YHg0wcstrr1dKtzfGOtqV3nmRr8vhIQ3oJa60 gCiOeVVCHNFSFuhiE3C8dKEJ4JMTXqrg1zre6EpM8Bgr5rW2UNViyQ5CTHCYhVdmJxIVLdV8mEFcx TWLS2FHrw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k0n6v-0005vC-PB; Wed, 29 Jul 2020 14:31:13 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k0n6s-0005uR-Du for linux-arm-kernel@lists.infradead.org; Wed, 29 Jul 2020 14:31:11 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B526030E; Wed, 29 Jul 2020 07:31:05 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 610063F66E; Wed, 29 Jul 2020 07:31:04 -0700 (PDT) Date: Wed, 29 Jul 2020 15:31:02 +0100 From: Dave Martin To: Suzuki K Poulose Subject: Re: [PATCH v4 2/4] arm64: cpufeature: Modify address authentication cpufeature to exact Message-ID: <20200729143101.GG21941@arm.com> References: <1594368010-4419-1-git-send-email-amit.kachhap@arm.com> <1594368010-4419-3-git-send-email-amit.kachhap@arm.com> <20200729103648.GC21941@arm.com> <66cc667d-7397-bbc7-bde1-c562964a7672@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <66cc667d-7397-bbc7-bde1-c562964a7672@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200729_103110_567318_B62FC6C9 X-CRM114-Status: GOOD ( 35.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, keescook@chromium.org, catalin.marinas@arm.com, broonie@kernel.org, james.morse@arm.com, amit.kachhap@arm.com, Vincenzo.Frascino@arm.com, will@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >>[Suzuki: Introduce new matching function for address authentication] > >>Suggested-by: Suzuki K Poulose > >>--- > >>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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel