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=-10.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,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 EF05DC433E2 for ; Mon, 14 Sep 2020 06:55:52 +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 96009204FD for ; Mon, 14 Sep 2020 06:55:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="L7nRzlpU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 96009204FD 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-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=v83y/Wzkzdrmk/QZEtCEAGaXwc6QsJViZZ8q1cWBySM=; b=L7nRzlpU/mawqE5KF5zm5r+UI yYO9ohrC3rwRWpzb0OP2mjxgCl+JYzpvJ3ST6pmYQEIw0weutqtIZcoNhZ71KdMiVsTp3qlTsStms A+2mTcsL5gllc93fvWK6NiRnvc+Hl/kj29CkVJYRTIg2NYf37BMtbaJFCc8BpYzFC/GV28j3To+z3 nOY75xA8qQI+Ll6vzpoaX0lrxtjSa8qikMcC+tCqQ+mKbAyrgtkRnbrWiOmficjjW0OOfEAiSCrnI fNNrauxxjOpuM2QXQsDsAtntAbaLwWHxVZubM3W1xwzyFALHWZjEozp0vrVKw1/Az0rjkCW1AX3vj CcI3QtIHg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kHiNp-00074j-6L; Mon, 14 Sep 2020 06:54:37 +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 1kHiNl-00074C-FA for linux-arm-kernel@lists.infradead.org; Mon, 14 Sep 2020 06:54:34 +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 B8867D6E; Sun, 13 Sep 2020 23:54:28 -0700 (PDT) Received: from [10.57.6.189] (unknown [10.57.6.189]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0FB863F73B; Sun, 13 Sep 2020 23:54:25 -0700 (PDT) Subject: Re: [PATCH v7 4/6] arm64: cpufeature: Modify address authentication cpufeature to exact To: Catalin Marinas References: <20200909140759.4170-1-amit.kachhap@arm.com> <20200909140759.4170-5-amit.kachhap@arm.com> <20200911172418.GL12835@gaia> From: Amit Kachhap Message-ID: <8128f714-0f99-1113-de41-03d35b89c5b6@arm.com> Date: Mon, 14 Sep 2020 12:24:23 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20200911172418.GL12835@gaia> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200914_025433_603398_ED75EEC6 X-CRM114-Status: GOOD ( 40.21 ) 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 , Suzuki K Poulose , Mark Brown , James Morse , Vincenzo Frascino , Will Deacon , Dave Martin , linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 9/11/20 10:54 PM, Catalin Marinas wrote: > On Wed, Sep 09, 2020 at 07:37:57PM +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. >> >> Following ptrauth crash log is oberserved in Arm fastmodel with mismatched >> cpus without this fix, >> >> CPU features: SANITY CHECK: Unexpected variation in SYS_ID_AA64ISAR1_EL1. Boot CPU: 0x11111110211402, CPU4: 0x11111110211102 >> CPU features: Unsupported CPU feature variation detected. >> GICv3: CPU4: found redistributor 100 region 0:0x000000002f180000 >> CPU4: Booted secondary processor 0x0000000100 [0x410fd0f0] >> Unable to handle kernel paging request at virtual address bfff800010dadf3c >> Mem abort info: >> ESR = 0x86000004 >> EC = 0x21: IABT (current EL), IL = 32 bits >> SET = 0, FnV = 0 >> EA = 0, S1PTW = 0 >> [bfff800010dadf3c] address between user and kernel address ranges >> Internal error: Oops: 86000004 [#1] PREEMPT SMP >> Modules linked in: >> CPU: 4 PID: 29 Comm: migration/4 Tainted: G S 5.8.0-rc4-00005-ge658591d66d1-dirty #158 >> Hardware name: Foundation-v8A (DT) >> pstate: 60000089 (nZCv daIf -PAN -UAO BTYPE=--) >> pc : 0xbfff800010dadf3c >> lr : __schedule+0x2b4/0x5a8 >> sp : ffff800012043d70 >> x29: ffff800012043d70 x28: 0080000000000000 >> x27: ffff800011cbe000 x26: ffff00087ad37580 >> x25: ffff00087ad37000 x24: ffff800010de7d50 >> x23: ffff800011674018 x22: 0784800010dae2a8 >> x21: ffff00087ad37000 x20: ffff00087acb8000 >> x19: ffff00087f742100 x18: 0000000000000030 >> x17: 0000000000000000 x16: 0000000000000000 >> x15: ffff800011ac1000 x14: 00000000000001bd >> x13: 0000000000000000 x12: 0000000000000000 >> x11: 0000000000000000 x10: 71519a147ddfeb82 >> x9 : 825d5ec0fb246314 x8 : ffff00087ad37dd8 >> x7 : 0000000000000000 x6 : 00000000fffedb0e >> x5 : 00000000ffffffff x4 : 0000000000000000 >> x3 : 0000000000000028 x2 : ffff80086e11e000 >> x1 : ffff00087ad37000 x0 : ffff00087acdc600 >> Call trace: >> 0xbfff800010dadf3c >> schedule+0x78/0x110 >> schedule_preempt_disabled+0x24/0x40 >> __kthread_parkme+0x68/0xd0 >> kthread+0x138/0x160 >> ret_from_fork+0x10/0x34 >> Code: bad PC value > > That's what FTR_EXACT gives us. The variation above is in the field at > bit position 8 (API_SHIFT) with the boot CPU value of 4 and the > secondary CPU of 1, if I read it correctly. Yes > > Would it be better if the incompatible CPUs are just parked? I'm trying > to figure out from the verify_local_cpu_caps() code whether that's > possible. I don't fully understand why we don't trigger the "Detected > conflict for capability" message instead. The above ptrauth crash appears when this fix patch is not present and with this fix present, cpu4 is actually parked as, [ 0.098833] CPU features: CPU4: Detected conflict for capability 39 (Address authentication (IMP DEF algorithm)), System: 1, CPU: 0 [ 0.098833] CPU4: will not boot > > My reading of the code is that we set the system capability based on the > boot CPU since it's a BOOT_CPU_FEATURE. has_address_auth_cpucap() should > return false with this mismatch and verify_local_cpu_caps() would park > the CPU. Once parked, it shouldn't reach the sanity check since > cpuinfo_store_cpu() is called after check_local_cpu_capabilities() (and > the match function). Yes. > >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index 6424584be01e..4bb3f2b2ffed 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -197,9 +197,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), > > So we go for FTR_EXACT here with a safe value of 0. It makes sense. > IIUC, in case of a mismatch, we end up with the sanitised register field > as 0. > >> ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64ISAR1_DPB_SHIFT, 4, 0), >> ARM64_FTR_END, >> }; >> @@ -1648,11 +1648,39 @@ 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 boot_val, sec_val; >> + >> + /* We don't expect to be called with SCOPE_SYSTEM */ >> + WARN_ON(scope == SCOPE_SYSTEM); >> + /* >> + * The ptr-auth feature levels are not intercompatible with lower >> + * levels. Hence we must match ptr-auth feature level of the secondary >> + * CPUs with that of the boot CPU. The level of boot cpu is fetched >> + * from the sanitised register whereas direct register read is done for >> + * the secondary CPUs. >> + * The sanitised feature state is guaranteed to match that of the >> + * boot CPU as a mismatched secondary CPU is parked before it gets >> + * a chance to update the state, with the capability. >> + */ >> + boot_val = cpuid_feature_extract_field(read_sanitised_ftr_reg(entry->sys_reg), >> + entry->field_pos, entry->sign); >> + if (scope & SCOPE_BOOT_CPU) { >> + return boot_val >= entry->min_field_value; >> + } else if (scope & SCOPE_LOCAL_CPU) { > > Nitpick: just drop the else as we had a return already above. We could > also drop the SCOPE_LOCAL_CPU check since that's the only one left. Ok I will update it in v8 version. > >> + sec_val = cpuid_feature_extract_field(__read_sysreg_by_encoding(entry->sys_reg), >> + entry->field_pos, entry->sign); >> + return (sec_val >= entry->min_field_value) && (sec_val == boot_val); > > Another nitpick: do you still need the sec_val >= ... if you check for > sec_val == boot_val? boot_val was already checked against > min_field_value. That's unless the sanitised reg is updated and boot_val > above is 0 on subsequent CPUs. This could only happen if we don't park a > CPU that has a mismatch and it ends up updating the sysreg. AFAICT, for > a secondary CPU, we first check the features then we update the > sanitised regs. Yes agreed. I will update it in v8 version. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel