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=-5.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,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 A8561C10DCE for ; Fri, 6 Mar 2020 19:07:21 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 7012620675 for ; Fri, 6 Mar 2020 19:07:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ikZLl6Rm" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7012620675 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+infradead-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=bombadil.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: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=0qtzyIwJ+NZV9KPnvlB8xGGlWML1XpysU2ueS5344ik=; b=ikZLl6RmKnXZYM ikXwIEA1D8H41ccmy/7uk6MFu8DTiNs0jRnUCbxA+oOkInZCzWF2sNhcgXMRCGm6BP28HZwKg7I4c CykyJ7226DBnOr3MN4ggPNJfQIXFvNev1kxQ13whdlBQAfG7hHQaM6JhjS3JGPfL/HIW23diaSCG8 xJ0T6IFArxG3dOKEFxaPwPmKoSgy75jCWKsxdQwIBgF7RoeAiOYct8K+LM4FyG9zdO7Yf1pXl8DfQ p0dOPOpxKSAwEf3y4QvzXWxuq/jvzbuUYWOiZRvLqaUhd2tVF+H39rc9R5VtIBwKIaq8rwoaz/Vv9 /NLEtIXABfs+iRH1RzdQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jAIJc-0007he-SX; Fri, 06 Mar 2020 19:07:20 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jAIJa-0007hE-3M for linux-arm-kernel@lists.infradead.org; Fri, 06 Mar 2020 19:07:19 +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 00C731FB; Fri, 6 Mar 2020 11:07:14 -0800 (PST) Received: from [10.1.196.105] (eglon.cambridge.arm.com [10.1.196.105]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4DBB43F6C4; Fri, 6 Mar 2020 11:07:13 -0800 (PST) Subject: Re: [PATCH v6 12/18] arm64: mask PAC bits of __builtin_return_address To: Amit Daniel Kachhap References: <1583476525-13505-1-git-send-email-amit.kachhap@arm.com> <1583476525-13505-13-git-send-email-amit.kachhap@arm.com> From: James Morse Message-ID: <6171e139-cc10-53b3-1121-0053fc947b49@arm.com> Date: Fri, 6 Mar 2020 19:07:11 +0000 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <1583476525-13505-13-git-send-email-amit.kachhap@arm.com> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200306_110718_230661_CDB9C3A7 X-CRM114-Status: GOOD ( 23.86 ) 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 , Kees Cook , Suzuki K Poulose , Catalin Marinas , Kristina Martsenko , Dave Martin , Mark Brown , Ramana Radhakrishnan , Vincenzo Frascino , Will Deacon , Ard Biesheuvel , 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+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Amit, On 06/03/2020 06:35, Amit Daniel Kachhap wrote: > This redefines __builtin_return_address to mask pac bits > when Pointer Authentication is enabled. As __builtin_return_address > is used mostly used to refer to the caller function symbol address > so masking runtime generated pac bits will help to find the match. I'm not entirely sure what problem you are trying to solve from this paragraph. > This patch adds a new file (asm/compiler.h) and is transitively > included (via include/compiler_types.h) on the compiler command line > so it is guaranteed to be loaded and the users of this macro will > not find a wrong version. > > A helper macro ptrauth_kernel_pac_mask is created for this purpose > and added in this file. A similar macro ptrauth_user_pac_mask exists > in pointer_auth.h and is now moved here for the sake of consistency. > This change fixes the utilities like cat /proc/vmallocinfo to show > correct symbol names. This is to avoid things like: | 0x(____ptrval____)-0x(____ptrval____) 20480 0xb8b8000100f75fc pages=4 vmalloc N0=4 | 0x(____ptrval____)-0x(____ptrval____) 20480 0xc0f28000100f75fc pages=4 vmalloc N0=4 Where those 64bit values should be the same symbol name, not different LR values. Could you phrase the commit message to describe the problem, then how you fix it. something like: | Functions like vmap() record how much memory has been allocated by their callers, | callers are identified using __builtin_return_address(). Once the kernel is using | pointer-auth the return address will be signed. This means it will not match any kernel | symbol, and will vary between threads even for the same caller. | | Use the pre-processor to add logic to strip the PAC to __builtin_return_address() | callers. > diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h > new file mode 100644 > index 0000000..085e7cd0 > --- /dev/null > +++ b/arch/arm64/include/asm/compiler.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __ASM_COMPILER_H > +#define __ASM_COMPILER_H > + > +#if defined(CONFIG_ARM64_PTR_AUTH) > + > +/* > + * The EL0/EL1 pointer bits used by a pointer authentication code. > + * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply. > + */ > +#define ptrauth_user_pac_mask() GENMASK_ULL(54, vabits_actual) > +#define ptrauth_kernel_pac_mask() GENMASK_ULL(63, vabits_actual) > + > +#define __builtin_return_address(val) \ > + (void *)((unsigned long)__builtin_return_address(val) | \ > + ptrauth_kernel_pac_mask()) This is pretty manky, ideally we would have some __arch_return_address() hook for this, but this works, and lets us postpone any tree-wide noise until its absolutely necessary. (I assume if this does ever break, it will be a build error) You add ptrauth_strip_insn_pac() in this patch, but don't use it until the next one. However... could you use it here? This would go wrong if __builtin_return_address() legitimately returned a value that was mapped by TTBR0, we would force the top bits to be set instead of clearing the PAC bits. This would corrupt the value instead of corrupting it. I don't think there is anywhere this could happen today, but passing callbacks into UEFI runtime services or making kernel calls from an idmap may both do this. With that: Reviewed-by: James Morse Thanks! James > --- a/arch/arm64/include/asm/pointer_auth.h > +++ b/arch/arm64/include/asm/pointer_auth.h > @@ -68,16 +68,13 @@ 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); > > -/* > - * The EL0 pointer bits used by a pointer authentication code. > - * This is dependent on TBI0 being enabled, or bits 63:56 would also apply. > - */ > -#define ptrauth_user_pac_mask() GENMASK(54, vabits_actual) > - > -/* Only valid for EL0 TTBR0 instruction pointers */ > +/* Valid for EL0 TTBR0 and EL1 TTBR1 instruction pointers */ > static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr) > { > - return ptr & ~ptrauth_user_pac_mask(); > + if (ptr & BIT_ULL(55)) > + return ptr | ptrauth_kernel_pac_mask(); > + else > + return ptr & ~ptrauth_user_pac_mask(); > } > > #define ptrauth_thread_init_user(tsk) \ > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel