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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7326AC433FE for ; Tue, 8 Nov 2022 14:42:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=QebRINgw6zqXxqcc74uWIvaNAkbo14w0WleGIEMRpV4=; b=EyuieVKnZZHwn5 zqej6z1g6bS0lwb64Khpb3ZJeJ062ilnmRO0xMKMa6OHTuLlmQIDEoyIuXZ98jEyYi0n14oABDEsS IBTaDfnQ9rMLeTyF8WL1a5Ykn4Z8VCOmzj2HBSNOFDKQF5xUNE28ttNnuFAZ0KTBnCBs0W/QY6Rcg ge4e3mPoH3eZmGPCJLH6tZcAIIm821oOBlyW9XpVqyZhxjikLX+e6OFdT1l25jMBrgmFsVLR4lzrK eIkDM5H8gR+7ywBo8OOl/8D7GplNLVD5mfVDmrDcdMzPNjAyXRxwO6KUzwbJ/wk0ppkfpymM4fCBF JzeiEuY1rtCCiTFWV/bQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1osPnH-005y2i-VT; Tue, 08 Nov 2022 14:41:40 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1osPnE-005y2J-Mo for linux-arm-kernel@lists.infradead.org; Tue, 08 Nov 2022 14:41:38 +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 E65741FB; Tue, 8 Nov 2022 06:41:40 -0800 (PST) Received: from FVFF77S0Q05N.cambridge.arm.com (FVFF77S0Q05N.cambridge.arm.com [10.1.38.153]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 42A683F73D; Tue, 8 Nov 2022 06:41:33 -0800 (PST) Date: Tue, 8 Nov 2022 14:41:26 +0000 From: Mark Rutland To: Ard Biesheuvel Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon , Marc Zyngier , Eric Biggers , "Jason A . Donenfeld" , Kees Cook , Suzuki K Poulose , Adam Langley Subject: Re: [PATCH v2] arm64: Enable data independent timing (DIT) in the kernel Message-ID: References: <20221107172400.1851434-1-ardb@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221107172400.1851434-1-ardb@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221108_064136_858945_35F708C2 X-CRM114-Status: GOOD ( 28.82 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 Hi Ard, On Mon, Nov 07, 2022 at 06:24:00PM +0100, Ard Biesheuvel wrote: > The ARM architecture revision v8.4 introduces a data independent timing > control (DIT) which can be set at any exception level, and instructs the > CPU to avoid optimizations that may result in a correlation between the > execution time of certain instructions and the value of the data they > operate on. > > The DIT bit is part of PSTATE, and is therefore context switched as > usual, given that it becomes part of the saved program state (SPSR) when > taking an exception. We have also defined a hwcap for DIT, and so user > space can discover already whether or nor DIT is available. This means > that, as far as user space is concerned, DIT is wired up and fully > functional. > > In the kernel, however, we never bothered with DIT: we disable at it > boot (i.e., INIT_PSTATE_EL1 has DIT cleared) and ignore the fact that we > might run with DIT enabled if user space happened to set it. > > Currently, we have no idea whether or not running privileged code with > DIT disabled on a CPU that implements support for it may result in a > side channel that exposes privileged data to unprivileged user space > processes, so let's be cautious and just enable DIT while running in the > kernel if supported by all CPUs. Thanks for respinning the wording! I have one minor nit below, but otherwise this looks good to me. > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index f73f11b5504254be..f44579bca9f8107e 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -875,6 +875,11 @@ static inline bool cpu_has_pan(void) > ID_AA64MMFR1_EL1_PAN_SHIFT); > } > > +static inline bool cpu_has_dit(void) > +{ > + return cpus_have_const_cap(ARM64_HAS_DIT); > +} Normally cpu_has_X() implies a local feature check, and cpus_have_X() tests for common support, so this should be cpus_have_dit(). That said, this is only used in one place below, so we could use the CAP directly there without a wrapper. > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 7d301700d1a93692..1f3f52ce407fe942 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -90,20 +90,24 @@ > */ > #define pstate_field(op1, op2) ((op1) << Op1_shift | (op2) << Op2_shift) > #define PSTATE_Imm_shift CRm_shift > +#define SET_PSTATE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) > > #define PSTATE_PAN pstate_field(0, 4) > #define PSTATE_UAO pstate_field(0, 3) > #define PSTATE_SSBS pstate_field(3, 1) > +#define PSTATE_DIT pstate_field(3, 2) > #define PSTATE_TCO pstate_field(3, 4) > > -#define SET_PSTATE_PAN(x) __emit_inst(0xd500401f | PSTATE_PAN | ((!!x) << PSTATE_Imm_shift)) > -#define SET_PSTATE_UAO(x) __emit_inst(0xd500401f | PSTATE_UAO | ((!!x) << PSTATE_Imm_shift)) > -#define SET_PSTATE_SSBS(x) __emit_inst(0xd500401f | PSTATE_SSBS | ((!!x) << PSTATE_Imm_shift)) > -#define SET_PSTATE_TCO(x) __emit_inst(0xd500401f | PSTATE_TCO | ((!!x) << PSTATE_Imm_shift)) > +#define SET_PSTATE_PAN(x) SET_PSTATE((x), PAN) > +#define SET_PSTATE_UAO(x) SET_PSTATE((x), UAO) > +#define SET_PSTATE_SSBS(x) SET_PSTATE((x), SSBS) > +#define SET_PSTATE_DIT(x) SET_PSTATE((x), DIT) > +#define SET_PSTATE_TCO(x) SET_PSTATE((x), TCO) Nice! [...] > diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c > index 8b02d310838f9240..3032a82ea51a19f7 100644 > --- a/arch/arm64/kernel/suspend.c > +++ b/arch/arm64/kernel/suspend.c > @@ -60,6 +60,8 @@ void notrace __cpu_suspend_exit(void) > * PSTATE was not saved over suspend/resume, re-enable any detected > * features that might not have been set correctly. > */ > + if (cpu_has_dit()) > + set_pstate_dit(1); As above, I'd prefer if we either renamed cpu_has_dit() to cpus_have_dit(), or just open-coded this as: if (cpus_have_const_cap(ARM64_HAS_DIT)) set_pstate_dit(1); With either of those options: Acked-by: Mark Rutland I assume Will might fix that up when applying. Mark. > __uaccess_enable_hw_pan(); > > /* > diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps > index f1c0347ec31a85c7..a86ee376920a08dd 100644 > --- a/arch/arm64/tools/cpucaps > +++ b/arch/arm64/tools/cpucaps > @@ -20,6 +20,7 @@ HAS_CNP > HAS_CRC32 > HAS_DCPODP > HAS_DCPOP > +HAS_DIT > HAS_E0PD > HAS_ECV > HAS_EPAN > -- > 2.35.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel