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=-7.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 D0D14C433E1 for ; Thu, 27 Aug 2020 02:58:05 +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 919B32075B for ; Thu, 27 Aug 2020 02:58:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="IxYtiZ/+"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="d1in0VXz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 919B32075B Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.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:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WxmdGf/k15gDmK8yB80rcwBqHEo4IFPbOJM33yBy3gs=; b=IxYtiZ/+dpARis4qepPoy89v2 6Ta2pefGW9YezetYliEaGt92IT702swHeWCBNyn/g+Up7YEkKc7QSxf7q1n1EKB2LT9ZBq5r3M2Jh G+qF44EFEzTplBLOh2fnT0KvlNgmaVv7FPXI3dTLPlLopoduNcPTH3NNclq8nCfFsIlvZHV7ed7Sl jmVBcHALbKBQS0UlGs3lsXcnf+plBCMe/rdsPUJg3Nq4h8t/2HKr5sC7z90fMn5tK14Wzz6ScYqpD 1gd00AFG+cAjXf8jleIylouk7c5T8u30hbv/DuPNvJPVfMatPwliCUeDDTU6Tjj5mtkVSlY+YVsGS rpyVwgXJw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kB85G-0001XP-B5; Thu, 27 Aug 2020 02:56:14 +0000 Received: from mail-ua1-x92f.google.com ([2607:f8b0:4864:20::92f]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kB85D-0001X1-Ma for linux-arm-kernel@lists.infradead.org; Thu, 27 Aug 2020 02:56:13 +0000 Received: by mail-ua1-x92f.google.com with SMTP id v24so1227993uaj.7 for ; Wed, 26 Aug 2020 19:56:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qNj7wLV2nlflmQ86em773ZNoUeQQjZz8dTH1PTYBxrc=; b=d1in0VXzjtu7onRiCMhsnh0OYqvvrSNVYGxtQj5dFb3pb+VWUrRhUL94xpbURrgUPk euW8nxaPMEWwVot9xFuMCoepb7sQSHfKSSf+/Cjco9ZAZjX1V+X3h9DnnhUhmMwgaaYi mfdkaJiLLvBl6JXyvLFxCZC3WubzhGSkjnUPuTSmR4Ky83bAiLb1F77Z856jYZtTmA5m B78GhY+F6UECLhB6jd/tYgPTWgbmSdhv6TW9oELA5UgVirHRg+5PrjI4UOxjBAxzZO6W SKC7jKEu3GDK9te6khyy7UiyDyZAzMqu7WPUWKtv8LGP6qCK+tmdLcv+WEcYij8P+oYe T/ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qNj7wLV2nlflmQ86em773ZNoUeQQjZz8dTH1PTYBxrc=; b=cXK0q1KPzLlrvgP6OTc/xdNrG0ogGYG9VCkggqXKRQThOg1YBRSFPjTXkuHdaC4kRt gVqj+u5YvRL3Nr/206pYuHGOrFbh+b+B57GmGX8nXVcMqvuhquo1+WL0u1vFXERXoImd yGE8X8QdT/b83/HnJ7ZKvmKBED7d1z9xc8caHL5XhpNbJvcbKP4chZIZr48m13it+hOe 1wQKxcUtIXyG+B0wGz4Vv0qlgWS+n/KThBg3cFDEAEP2Y5Cy8L2Dok1CxMJ2F1z6XBtr /8snskqe6CMvHR2/SX5RfpDvDUutS5bXcNLk8EfLoKbsROpOvFhx+JFIbjBqE2Mxl/tM TwHQ== X-Gm-Message-State: AOAM530WeyDEIfornWA1GIBUjosnol5kw0JIYBtToU8J3MUhlio/V4kv 4QI5NnQQf6+n2T6VCtIPi0Ujcmbr8ZAzYFUIlqncJg== X-Google-Smtp-Source: ABdhPJwQKc7juu72uHCnAltUXuiip//YDhF2DuuwO4qrXpaY15KHpJeKFZOO3VNXXYYSN6UwRzJmT6hbvCVT+o2aaC8= X-Received: by 2002:ab0:2996:: with SMTP id u22mr10653291uap.108.1598496963339; Wed, 26 Aug 2020 19:56:03 -0700 (PDT) MIME-Version: 1.0 References: <20200801011152.39838-1-pcc@google.com> <20200819101809.GD6642@arm.com> <20200824144910.GO6642@arm.com> <20200826163726.GV6642@arm.com> In-Reply-To: <20200826163726.GV6642@arm.com> From: Peter Collingbourne Date: Wed, 26 Aug 2020 19:55:51 -0700 Message-ID: Subject: Re: [PATCH] arm64: add prctl(PR_PAC_SET_ENABLED_KEYS) To: Dave Martin X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200826_225611_820318_15D39705 X-CRM114-Status: GOOD ( 68.53 ) 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: Andrey Konovalov , Kevin Brodsky , Kostya Serebryany , Linux ARM , Catalin Marinas , Vincenzo Frascino , Will Deacon , Evgenii Stepanov 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, Aug 26, 2020 at 9:37 AM Dave Martin wrote: > > On Mon, Aug 24, 2020 at 05:23:27PM -0700, Peter Collingbourne wrote: > > On Mon, Aug 24, 2020 at 7:49 AM Dave Martin wrote: > > > > > > On Wed, Aug 19, 2020 at 02:25:45PM -0700, Peter Collingbourne wrote: > > > > On Wed, Aug 19, 2020 at 3:18 AM Dave Martin wrote: > > > > > > > > > > On Fri, Jul 31, 2020 at 06:11:52PM -0700, Peter Collingbourne wrote: > > > > > > This prctl allows the user program to control which PAC keys are enabled > > > > > > in a particular task. The main reason why this is useful is to enable a > > > > > > userspace ABI that uses PAC to sign and authenticate function pointers > > > > > > and other pointers exposed outside of the function, while still allowing > > > > > > binaries conforming to the ABI to interoperate with legacy binaries that > > > > > > do not sign or authenticate pointers. > > > > > > > > > > > > The idea is that a dynamic loader or early startup code would issue > > > > > > this prctl very early after establishing that a process may load legacy > > > > > > binaries, but before executing any PAC instructions. > > > > > > > > > > Apologies for the slow response on this, I'd had it on my list for a > > > > > while... > > > > > > > > > > > --- > > > > > > .../arm64/pointer-authentication.rst | 27 +++++++++++++++ > > > > > > arch/arm64/include/asm/asm_pointer_auth.h | 19 +++++++++++ > > > > > > arch/arm64/include/asm/pointer_auth.h | 10 ++++-- > > > > > > arch/arm64/include/asm/processor.h | 5 +++ > > > > > > arch/arm64/kernel/asm-offsets.c | 1 + > > > > > > arch/arm64/kernel/pointer_auth.c | 34 +++++++++++++++++++ > > > > > > include/uapi/linux/prctl.h | 3 ++ > > > > > > kernel/sys.c | 8 +++++ > > > > > > 8 files changed, 105 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/arm64/pointer-authentication.rst b/Documentation/arm64/pointer-authentication.rst > > > > > > index 30b2ab06526b..1f7e064deeb3 100644 > > > > > > --- a/Documentation/arm64/pointer-authentication.rst > > > > > > +++ b/Documentation/arm64/pointer-authentication.rst > > > > > > @@ -107,3 +107,30 @@ filter out the Pointer Authentication system key registers from > > > > > > KVM_GET/SET_REG_* ioctls and mask those features from cpufeature ID > > > > > > register. Any attempt to use the Pointer Authentication instructions will > > > > > > result in an UNDEFINED exception being injected into the guest. > > > > > > + > > > > > > + > > > > > > +Enabling and disabling keys > > > > > > +--------------------------- > > > > > > + > > > > > > +The prctl PR_PAC_SET_ENABLED_KEYS allows the user program to control which > > > > > > +PAC keys are enabled in a particular task. It takes two arguments, the > > > > > > +first being a bitmask of PR_PAC_APIAKEY, PR_PAC_APIBKEY, PR_PAC_APDAKEY > > > > > > +and PR_PAC_APDBKEY specifying which keys shall be affected by this prctl, > > > > > > +and the second being a bitmask of the same bits specifying whether the key > > > > > > +should be enabled or disabled. For example:: > > > > > > + > > > > > > + prctl(PR_PAC_SET_ENABLED_KEYS, > > > > > > + PR_PAC_APIAKEY | PR_PAC_APIBKEY | PR_PAC_APDAKEY | PR_PAC_APDBKEY, > > > > > > + PR_PAC_APIBKEY, 0, 0); > > > > > > + > > > > > > +disables all keys except the IB key. > > > > > > + > > > > > > +The main reason why this is useful is to enable a userspace ABI that uses PAC > > > > > > +instructions to sign and authenticate function pointers and other pointers > > > > > > +exposed outside of the function, while still allowing binaries conforming to > > > > > > +the ABI to interoperate with legacy binaries that do not sign or authenticate > > > > > > +pointers. > > > > > > > > > > What actually breaks without this? > > > > > > > > > > Since the keys are all enabled by default, the only purpose of this > > > > > prctl seems to be to disable keys. I'm not sure what this is giving us. > > > > > > > > Yes, the purpose is to disable keys. Let's consider the function > > > > pointer signing userspace ABI use case. An example is Apple's arm64e > > > > ABI, and I have a prototype branch of LLVM [0] that implements a > > > > similar ABI in Linux userspace, based on Apple's implementation of > > > > their ABI. > > > > > > > > Here's an example of a function that returns a function pointer, and a > > > > function that calls a function pointer of the same type: > > > > > > > > static void f(void) {} > > > > > > > > void *return_fp(void) { > > > > return f; > > > > } > > > > > > > > void call_fp(void (*p)(void)) { > > > > p(); > > > > } > > > > > > > > If I compile this with my prototype compiler I get: > > > > > > > > $ clang --target=aarch64-android -fptrauth-calls fptr.c -S -o - -O3 > > > > -march=armv8.3a > > > > [...] > > > > return_fp: // @return_fp > > > > // %bb.0: // %entry > > > > adrp x16, f > > > > add x16, x16, :lo12:f > > > > mov x17, #16277 > > > > pacia x16, x17 > > > > mov x0, x16 > > > > ret > > > > [...] > > > > call_fp: // @call_fp > > > > // %bb.0: // %entry > > > > mov w1, #16277 > > > > braa x0, x1 > > > > [...] > > > > > > > > In this code snippet the function pointer is signed with the IA key > > > > and discriminator 16277 before being returned. When the function is > > > > called, the pointer is first authenticated with the same key and > > > > discriminator. > > > > > > > > Now imagine that this code lives in a shared library used both by > > > > programs that use the function pointer signing ABI and by legacy > > > > binaries (i.e. programs that use the existing ABI), and we have a > > > > legacy binary that calls return_fp. If the legacy binary then calls > > > > the function pointer returned by return_fp, that code will not > > > > authenticate the pointer before calling it, it will just use a br or > > > > blr instruction to call it directly, which will lead to a crash if the > > > > signature bits are set in the function pointer. In order to prevent > > > > the crash, we need a way to cause the pacia instruction in return_fp > > > > to become a no-op when running inside the process hosting the legacy > > > > binary, so that the signature bits will remain clear and the br or blr > > > > instruction in the legacy binary will successfully call the function > > > > f. That can be done by disabling the IA key, which is exactly what > > > > this prctl() lets us do. > > > > > > Ack, I think there has been past discussion on this, but it's been > > > waiting for a usecase since it does impose a bit of extra overhead. > > > > > > (I'd somehow assumes that you were actually wanting to get SIGILLs, > > > rather then compatibly executing some ptrauth insns as NOPs -- the > > > latter makes much more sense.) > > > > > > > > > Is this going to land for real, or is it just something people have > > > been experimenting with? > > > > > > (I can guess the answer from the fact that you've proposed this patch > > > in the first place, but I have to ask...) > > > > It's something that we're considering for a future revision of the > > Android ABI, and something that I'm personally interested in having, > > but no final decision has been made. > > > > One problem is that by the time we're ready to make a decision, it'll > > probably be far too late to make a start on getting the necessary > > kernel support added, hence the desire to get started on this early. > > Fair enough. It's far from being pure fantasy at least, and the use > case seems sufficiently real. > > > > > > > > > > > > > > > > > > + > > > > > > +The idea is that a dynamic loader or early startup code would issue this > > > > > > +prctl very early after establishing that a process may load legacy binaries, > > > > > > +but before executing any PAC instructions. > > > > > > diff --git a/arch/arm64/include/asm/asm_pointer_auth.h b/arch/arm64/include/asm/asm_pointer_auth.h > > > > > > index 52dead2a8640..d121fa5fed5f 100644 > > > > > > --- a/arch/arm64/include/asm/asm_pointer_auth.h > > > > > > +++ b/arch/arm64/include/asm/asm_pointer_auth.h > > > > > > @@ -31,6 +31,14 @@ alternative_else_nop_endif > > > > > > ldp \tmp2, \tmp3, [\tmp1, #PTRAUTH_USER_KEY_APDB] > > > > > > msr_s SYS_APDBKEYLO_EL1, \tmp2 > > > > > > msr_s SYS_APDBKEYHI_EL1, \tmp3 > > > > > > + > > > > > > + ldr \tmp2, [\tsk, #THREAD_SCTLR_ENXX_MASK] > > > > > > + cbz \tmp2, .Laddr_auth_skip_\@ > > > > > > > > > > I wonder whether it would make sense to simple store the thread's base > > > > > SCTLR value (containing the ENxx bits), rather than storing the ENxx > > > > > bits separately. There may be reasons outside this snippet why this > > > > > isn't such a good idea though -- I haven't gone digging so far. > > > > > > > > As far as I know (as I learned [1] from the MTE patch series), it can > > > > be expensive to access SCTLR_EL1, therefore I arrange to only access > > > > SCTLR_EL1 in the hopefully-uncommon case where a process has disabled > > > > keys. Detecting the "process has disabled keys" case is quite simple > > > > if we only store the disabled keys mask here, not so much if we store > > > > the full value of SCTLR_EL1. > > > > > > My thought was that we would still only write SCTLR_EL1 if needed, but > > > we would do the write-if-needed across the whole register in one go. > > > This would be easier to extend if we have to twiddle additional > > > SCTLR_EL1 bits in the future. If the key enable bits are the only > > > affected bits for now then we could of course defer this factoring until > > > later. (I vaguely remember a similar discussion in the past, but > > > possibly it was about the pauth keys anyway, rather than something > > > else.) > > > > We will also need a per-task SCTLR_EL1.TCF0 setting for MTE. We could > > potentially combine the two, so that both > > prctl(PR_PAC_SET_ENABLED_KEYS) and prctl(PR_SET_TAGGED_ADDR_CTRL) end > > up affecting a common SCTLR_EL1 field in the task_struct, and we do: > > > > On kernel entry: > > - read SCTLR_EL1 from task_struct (or from the system register if we > > decide that's cheap enough) > > - if EnIA clear, set it, and write the value to the system register. > > > > On kernel exit: > > - read system register SCTLR_EL1 > > - read SCTLR_EL1 from task_struct > > - if they are different, write task's SCTLR_EL1 to the system register. > > > > But this would require at least an unconditional read of SCTLR_EL1 per > > kernel exit. The comment that I referenced says that "accesses" to the > > register are expensive. I was operating under the assumption that both > > reads and writes are expensive, but if it's actually only writes that > > are expensive, we may be able to get away with the above. > > The kernel is in full control over what's in SCTLR_EL1, so perhaps we > could cache what we wrote percpu, if the overhead of reading it is > problematic. Maybe, but that sounds like the kind of complexity that I'd like to avoid. I went ahead and did some performance measurements using the following program, which issues 10M invalid syscalls and exits. This should give us something as close as possible to a measurement of a kernel entry and exit on its own. .globl _start _start: movz x1, :abs_g1:10000000 movk x1, :abs_g0_nc:10000000 .Lloop: mov x8, #65536 // invalid svc #0 sub x1, x1, #1 cbnz x1, .Lloop mov x0, #0 mov x8, #0x5d // exit svc #0 On a Pixel 4 (which according to Wikipedia has a CPU based on Cortex-A76/A55) I measured the median of 1000 runs execution time at 0.554511s, implying an entry/exit cost of 55.5ns. If I make this modification to the kernel (Pixel 4 uses kernel version 4.14 so this won't apply cleanly to modern kernels; this is in the same place as the ptrauth_keys_install_user macro invocation in a modern kernel): diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 303447c765f7..b7c72d767f3e 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -340,6 +340,7 @@ alternative_else_nop_endif #endif 3: apply_ssbd 0, 5f, x0, x1 + mrs x0, sctlr_el1 5: .endif The median execution time goes to 0.565604s, implying a cost of 1.1ns to read the system register. I also measured the cost of reading from a percpu variable at kernel exit like so: diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 303447c765f7..c5a89785f650 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -340,6 +340,7 @@ alternative_else_nop_endif #endif 3: apply_ssbd 0, 5f, x0, x1 + ldr_this_cpu x0, sctlr_el1_cache, x1 5: .endif diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index 0b6ca5479f1f..ac9c9a6179d4 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -52,6 +52,8 @@ #include #include +DEFINE_PER_CPU(unsigned long, sctlr_el1_cache); + static const char *handler[]= { "Synchronous Abort", "IRQ", With that the median execution time goes to 0.600186s, implying a cost of 4.5ns. So at least on existing hardware, it looks like reading SCTLR_EL1 directly is probably our cheapest option if we're going to read *something*. Therefore I'll look at implementing this with unconditional reads from SCTLR_EL1 in v2. > > I could try to measure the cost of an unconditional read from > > SCTLR_EL1 on kernel exit on the hardware that I have access to, but I > > don't know if that's going to be representative of all hardware, > > especially the future hardware that will support PAC and MTE. > > > > > In a case like this, we'll still get overheads if there are a mixture of > > > tasks contending for the CPU, that have different key enable settings. > > > But I can't see much that we can do about that. If userspace is mostly > > > built with the same options (true for the Apple case I guess) then I > > > guess we shouldn't need SCTLR_EL1 rewrites very often just for this. In > > > other environments it may be messier. > > > > Yes, I don't think we can avoid the write when switching between tasks > > with different key enabled settings. If userspace arranges to use IA > > for return addresses (matching the kernel) and IB for function > > pointers, there would be no need to disable IA for compatibility with > > legacy binaries, and so the kernel would not require a write on entry, > > only on exit when transitioning between different settings. The effect > > is that disabling IA would be more expensive than disabling the other > > keys. > > > > > > > > > > > > + > > > > > > + mrs_s \tmp3, SYS_SCTLR_EL1 > > > > > > + bic \tmp3, \tmp3, \tmp2 > > > > > > + msr_s SYS_SCTLR_EL1, \tmp3 > > > > > > + > > > > > > .Laddr_auth_skip_\@: > > > > > > alternative_if ARM64_HAS_GENERIC_AUTH > > > > > > ldp \tmp2, \tmp3, [\tmp1, #PTRAUTH_USER_KEY_APGA] > > > > > > @@ -45,6 +53,17 @@ alternative_else_nop_endif > > > > > > ldp \tmp2, \tmp3, [\tmp1, #PTRAUTH_KERNEL_KEY_APIA] > > > > > > msr_s SYS_APIAKEYLO_EL1, \tmp2 > > > > > > msr_s SYS_APIAKEYHI_EL1, \tmp3 > > > > > > + > > > > > > + ldr \tmp2, [\tsk, #THREAD_SCTLR_ENXX_MASK] > > > > > > + cbz \tmp2, .Lset_sctlr_skip_\@ > > > > > > + > > > > > > + mrs_s \tmp1, SYS_SCTLR_EL1 > > > > > > + mov \tmp2, #(SCTLR_ELx_ENIA | SCTLR_ELx_ENIB | SCTLR_ELx_ENDA) > > > > > > > > > > (Nit: harmless but unnecessary (). # is not an operator as such, just > > > > > random syntax. Whatever follows is greedily parsed as an immediate > > > > > expression.) > > > > > > > > Okay. While looking around in the kernel I noticed that there is a > > > > mov_q macro that can be used to avoid manually splitting the constant > > > > into 16-bit chunks, and apparently it doesn't require a #. I'll use it > > > > in v2. > > > > > > > > > > + movk \tmp2, #SCTLR_ELx_ENDB > > > > > > > > > > Why do we check THREAD_SCTLR_ENXX_MASK, and then proceed to set all the > > > > > ENxx bits unconditionally? I may be missing something here. > > > > > > > > This code is to support the case where we are returning to the kernel > > > > from a userspace task with keys disabled. The kernel needs at least > > > > the IA key enabled in order for its own use of reverse-edge PAC to > > > > work correctly. When returning from a userspace task with no keys > > > > disabled, the keys enabled bits already have the correct values, so > > > > there is nothing to be done (and as mentioned above, I avoid touching > > > > SCTLR_EL1 unless necessary because it is apparently expensive to do > > > > so). But in a process with keys disabled, we will need to re-enable at > > > > least IA. > > > > > > > > We may be able to get away with just enabling IA here, but that would > > > > break the invariant that all keys are enabled inside the kernel, which > > > > is relied on by the code that decides whether to access SCTLR_EL1 in > > > > order to disable keys when entering a userspace task. > > > > > > OK, I think I just confused myself here: we are not setting the key > > > enables for userspace, but for the kernel, and we only need to do that > > > if the user task had some keys disabled in the first place. > > > > > > > > > > > > > + orr \tmp1, \tmp1, \tmp2 > > > > > > + msr_s SYS_SCTLR_EL1, \tmp1 > > > > > > + > > > > > > +.Lset_sctlr_skip_\@: > > > > > > .endm > > > > > > > > > > > > .macro ptrauth_keys_install_kernel_nosync tsk, tmp1, tmp2, tmp3 > > > > > > diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h > > > > > > index c6b4f0603024..d4c375454a36 100644 > > > > > > --- a/arch/arm64/include/asm/pointer_auth.h > > > > > > +++ b/arch/arm64/include/asm/pointer_auth.h > > > > > > @@ -70,14 +70,19 @@ static __always_inline void ptrauth_keys_switch_kernel(struct ptrauth_keys_kerne > > > > > > } > > > > > > > > > > > > extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg); > > > > > > +extern int ptrauth_prctl_set_enabled_keys(struct task_struct *tsk, > > > > > > + unsigned long keys, > > > > > > + unsigned long enabled); > > > > > > > > > > > > static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > > > > > > { > > > > > > return ptrauth_clear_pac(ptr); > > > > > > } > > > > > > > > > > > > -#define ptrauth_thread_init_user(tsk) \ > > > > > > - ptrauth_keys_init_user(&(tsk)->thread.keys_user) > > > > > > +#define ptrauth_thread_init_user(tsk) do { \ > > > > > > + ptrauth_keys_init_user(&(tsk)->thread.keys_user); \ > > > > > > + (tsk)->thread.sctlr_enxx_mask = 0; \ > > > > > > + } while (0) > > > > > > #define ptrauth_thread_init_kernel(tsk) \ > > > > > > ptrauth_keys_init_kernel(&(tsk)->thread.keys_kernel) > > > > > > #define ptrauth_thread_switch_kernel(tsk) \ > > > > > > @@ -85,6 +90,7 @@ static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > > > > > > > > > > > > #else /* CONFIG_ARM64_PTR_AUTH */ > > > > > > #define ptrauth_prctl_reset_keys(tsk, arg) (-EINVAL) > > > > > > +#define ptrauth_prctl_set_enabled_keys(tsk, keys, enabled) (-EINVAL) > > > > > > #define ptrauth_strip_insn_pac(lr) (lr) > > > > > > #define ptrauth_thread_init_user(tsk) > > > > > > #define ptrauth_thread_init_kernel(tsk) > > > > > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > > > > > > index 240fe5e5b720..6974d227b01f 100644 > > > > > > --- a/arch/arm64/include/asm/processor.h > > > > > > +++ b/arch/arm64/include/asm/processor.h > > > > > > @@ -150,6 +150,7 @@ struct thread_struct { > > > > > > #ifdef CONFIG_ARM64_PTR_AUTH > > > > > > struct ptrauth_keys_user keys_user; > > > > > > struct ptrauth_keys_kernel keys_kernel; > > > > > > + u64 sctlr_enxx_mask; > > > > > > #endif > > > > > > }; > > > > > > > > > > > > @@ -313,6 +314,10 @@ extern void __init minsigstksz_setup(void); > > > > > > /* PR_PAC_RESET_KEYS prctl */ > > > > > > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) > > > > > > > > > > > > +/* PR_PAC_SET_ENABLED_KEYS prctl */ > > > > > > +#define PAC_SET_ENABLED_KEYS(tsk, keys, enabled) \ > > > > > > + ptrauth_prctl_set_enabled_keys(tsk, keys, enabled) > > > > > > + > > > > > > #ifdef CONFIG_ARM64_TAGGED_ADDR_ABI > > > > > > /* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */ > > > > > > long set_tagged_addr_ctrl(unsigned long arg); > > > > > > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c > > > > > > index 0577e2142284..dac80e16fe35 100644 > > > > > > --- a/arch/arm64/kernel/asm-offsets.c > > > > > > +++ b/arch/arm64/kernel/asm-offsets.c > > > > > > @@ -47,6 +47,7 @@ int main(void) > > > > > > #ifdef CONFIG_ARM64_PTR_AUTH > > > > > > DEFINE(THREAD_KEYS_USER, offsetof(struct task_struct, thread.keys_user)); > > > > > > DEFINE(THREAD_KEYS_KERNEL, offsetof(struct task_struct, thread.keys_kernel)); > > > > > > + DEFINE(THREAD_SCTLR_ENXX_MASK,offsetof(struct task_struct, thread.sctlr_enxx_mask)); > > > > > > #endif > > > > > > BLANK(); > > > > > > DEFINE(S_X0, offsetof(struct pt_regs, regs[0])); > > > > > > diff --git a/arch/arm64/kernel/pointer_auth.c b/arch/arm64/kernel/pointer_auth.c > > > > > > index 1e77736a4f66..8c385b7f324a 100644 > > > > > > --- a/arch/arm64/kernel/pointer_auth.c > > > > > > +++ b/arch/arm64/kernel/pointer_auth.c > > > > > > @@ -42,3 +42,37 @@ int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg) > > > > > > > > > > > > return 0; > > > > > > } > > > > > > + > > > > > > +static u64 arg_to_enxx_mask(unsigned long arg) > > > > > > +{ > > > > > > + u64 sctlr_enxx_mask = 0; > > > > > > + if (arg & PR_PAC_APIAKEY) > > > > > > + sctlr_enxx_mask |= SCTLR_ELx_ENIA; > > > > > > + if (arg & PR_PAC_APIBKEY) > > > > > > + sctlr_enxx_mask |= SCTLR_ELx_ENIB; > > > > > > + if (arg & PR_PAC_APDAKEY) > > > > > > + sctlr_enxx_mask |= SCTLR_ELx_ENDA; > > > > > > + if (arg & PR_PAC_APDBKEY) > > > > > > + sctlr_enxx_mask |= SCTLR_ELx_ENDB; > > > > > > + return sctlr_enxx_mask; > > > > > > +} > > > > > > + > > > > > > +int ptrauth_prctl_set_enabled_keys(struct task_struct *tsk, unsigned long keys, > > > > > > + unsigned long enabled) > > > > > > +{ > > > > > > + u64 sctlr_enxx_mask = tsk->thread.sctlr_enxx_mask; > > > > > > + unsigned long addr_key_mask = PR_PAC_APIAKEY | PR_PAC_APIBKEY | > > > > > > + PR_PAC_APDAKEY | PR_PAC_APDBKEY; > > > > > > + > > > > > > + if (!system_supports_address_auth()) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + if ((keys & ~addr_key_mask) || (enabled & ~keys)) > > > > > > + return -EINVAL; > > > > > > > > > > Should we take the types of authentication supported? > > > > > > > > > > I don't recall whether we expose ptrauth to userspace if only > > > > > instruction authentication or only data authentication is supported. If > > > > > so, should we reject attempts to configure unsupported keys here? > > > > > > > > > > We should probably try to do a consistent thing both here and in > > > > > PR_PAC_RESET_KEYS if so. > > > > > > > > As far as I know, there is nothing in the architecture that would > > > > allow it to only advertise support for I keys or only advertise > > > > support for D keys. The fields AA64ISAR1_EL1.AP[AI] apply to all four > > > > keys: DA, DB, IA and IB. Maybe you are thinking of the GA key versus > > > > the other keys (which is advertised separately via > > > > AA64ISAR1_EL1.GP[AI])? The architecture appears to provide no way to > > > > disable the GA key, so I did not include support for it here. > > > > > > I think I'm confusing myself here. Yes, support for generic auth is > > > the (supposedly) architecturally orthogonal to address auth, but data > > > and instruction address auth are either both supported, or both not > > > supported -- so your code looks correct. > > > > > > > > > > > > > + > > > > > > + sctlr_enxx_mask |= arg_to_enxx_mask(keys); > > > > > > + sctlr_enxx_mask &= ~arg_to_enxx_mask(enabled); > > > > > > + > > > > > > + tsk->thread.sctlr_enxx_mask = sctlr_enxx_mask; > > > > > > + return 0; > > > > > > > > > > Do we need a way to query the enabled keys? > > > > > > > > > > We could either have a _GET_ prctl (the common approach), or have this > > > > > prctl return the mask of enabled keys (avoids the extra prctl, but > > > > > weirder). > > > > > > > > > > As above, we might > > > > > > > > > > Things like CRIU may need a GET in order to save/restore this setting > > > > > properly. > > > > > > > > Maybe it makes sense for there to be a GET prctl() to support CRIU. > > > > But it would need to be defined carefully because CRIU would > > > > presumably need to know what value to pass as the "keys" argument when > > > > it calls SET to restore the state. It can't just hardcode a value > > > > because that may harm extensibility if new keys are added. > > > > > > > > If we require the kernel to start processes with all keys enabled > > > > (including any keys that we may introduce in the future), then it > > > > seems like if CRIU knows which keys were disabled, then it can restore > > > > the state by issuing a syscall like this: > > > > > > > > prctl(PR_PAC_SET_ENABLED_KEYS, disabled_keys_mask, 0) > > > > > > > > which would imply that instead of providing PR_PAC_GET_ENABLED_KEYS we > > > > instead provide PR_PAC_GET_DISABLED_KEYS, which CRIU would call when > > > > saving the state to prepare the disabled_keys_mask argument passed > > > > here. Then for consistency we can make the SET prctl() be > > > > PR_PAC_SET_DISABLED_KEYS, and CRIU can then do: > > > > > > > > prctl(PR_PAC_SET_DISABLED_KEYS, disabled_keys_mask, disabled_keys_mask) > > > > > > > > Does that make sense? > > > > > > Possibly, though it's nicer if the GET returns the same value suppiled > > > to the SET, rather than the complement of it. > > > > Maybe you misread my proposal? The part about the complement is > > addressed by the sentence beginning "Then for consistency..." So we > > would end up with PR_PAC_SET_DISABLED_KEYS and > > PR_PAC_GET_DISABLED_KEYS, and they would both take/return a mask of > > *disabled* keys. > > Oh, I see. Yes, while I prefer the two are consistent with each other, > we can flip the sense of both here if desired. > > > > > > If SET ignores set bits in arg3 that don't correspond to set bits in the > > > mask arg2, then > > > > > > u64 current_keys = prctl(PR_PAC_GET_ENABLED_KEYS); > > > > > > prctl(PR_PAC_SET_ENABLED_KEYS, ~0UL, current_keys); > > > > > > should work. > > > > I'd prefer not to allow the SET prctl to accept all-ones as the first > > argument, as this could lead to a scenario where sometimes people pass > > all-ones as the first argument and sometimes they don't, which could > > make it hard to add new keys in the future (since the addition of a > > new key could cause existing callers to affect the new key > > arbitrarily). Instead, the first argument should be required to > > specify only known keys, in order to enforce that the caller is > > explicit about which keys they intend to affect. > > That sounds fair. > > > > There's a final option, which is to expose this config through ptrace > > > instead for save/restore purposes. From previous discussions with the > > > CRIU folks, I recall that they are trying to move away from doing setup > > > from within the new process where possible. > > > > > > There's no reason not to have both though -- there's precedent for that, > > > such as for PR_SVE_{GET,SET}_VL and the NT_ARM_SVE regset. MTE may move > > > in a similar direction too IIUC. > > > > > > Having a GET remains useful for in-process debugging and diagnostics, > > > and it's extremely straightforward to add in the kernel. So from my > > > side I'd vote to have it anyway... > > > > Okay. I see that the MTE series is adding NT_ARM_TAGGED_ADDR_CTRL as > > well, and I suppose that I could add a > > NT_ARM_PAC_(whatever)ABLED_KEYS. Since this wouldn't be used in normal > > userspace applications (and it doesn't look like ptrace APIs such as > > NT_ARM_PACA_KEYS will accommodate new keys being added so CRIU would > > need to be made aware of new keys anyway), we might as well just have > > all the APIs deal with the set of enabled keys. > > > > And if the ptrace is enough for CRIU, I'd mildly prefer not to add a > > GET prctl, since it's just more uapi surface that needs to be > > maintained forever. For in-process use cases it seems redundant with > > e.g. issuing a pacia instruction and checking whether it is a no-op, > > Probing instructions to see whether they generate signals is a cardinal > sin and we make huge efforts to ensure that userspace doesn't need to do > that. As for the no-op check, I suppose that works, but it's nasty if > the generic "check all the required featues" boilerplate on program > entry relies on intrinsics or can't be written in C at all. This is > just a way of discouraging people from checking at all IMHO. > > > The implementation of that prctl would most likely be a one-line wrapper > that calls the same internal function used to populate the regset for > ptrace, so I think you might be worrying a bit too much about creating > ABI here. This is more providing a second interface for same thing, > because no single interface is usable in all situations. Okay. Since I don't feel too strongly about it, I'll add the GET prctl in v2. Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel