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.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,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 F2489C433E0 for ; Tue, 26 Jan 2021 12:07:09 +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 ABBFF2311D for ; Tue, 26 Jan 2021 12:07:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ABBFF2311D Authentication-Results: mail.kernel.org; dmarc=fail (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=uF+xUnEKmyCmcJtfdeErsCgBl9nR0W0FcdkK5fkuEgA=; b=QI7IPe2OqNkm5028oI2YcQ++c sVhwcVlStwTnykjf+V6S9lR/xpQJfMfEJpD7q3h50/NIkxOXm8MUgmUMQrrfNmZhbZdfFTAAFyL7s Ii35EjY9MBPh+9gr/JV1FovrK+G9vPhrfTiyBTOw6mMh1kibJ2h8uUnHfHNR9l7RMhbQjg7tBiiQl Ets8kp1vgHiPv/i3ubdtoOSMB718v7PkW/uP+VkLkLZ3HHWe/qDS5icEFE4KVkskPhXeKIHRytA9d mp55CzLZxvHgnDz/1ar3mSW3eyhrNrmqM+2C55a04reu+UPU4oqtJeaV2PLYmTJcKFa/3nXKnLEyh oeZs44l7w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4N6U-0002qE-9r; Tue, 26 Jan 2021 12:05:50 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l4N6R-0002pk-BM for linux-arm-kernel@lists.infradead.org; Tue, 26 Jan 2021 12:05:48 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 4E4012311D; Tue, 26 Jan 2021 12:05:45 +0000 (UTC) Date: Tue, 26 Jan 2021 12:05:42 +0000 From: Catalin Marinas To: Will Deacon Subject: Re: [PATCH v3 1/2] arm64: Support execute-only permissions with Enhanced PAN Message-ID: <20210126120542.GA20158@gaia> References: <20210119160723.116983-1-vladimir.murzin@arm.com> <20210119160723.116983-2-vladimir.murzin@arm.com> <20210126110305.GA29467@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210126110305.GA29467@willie-the-truck> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210126_070547_533004_9C0D3486 X-CRM114-Status: GOOD ( 24.80 ) 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: Vladimir Murzin , keescook@chromium.org, linux-arm-kernel@lists.infradead.org, dave.martin@arm.com 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 Tue, Jan 26, 2021 at 11:03:06AM +0000, Will Deacon wrote: > On Tue, Jan 19, 2021 at 04:07:22PM +0000, Vladimir Murzin wrote: > > #define pte_valid_not_user(pte) \ > > - ((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID) > > + ((pte_val(pte) & (PTE_VALID | PTE_USER | PTE_UXN)) == (PTE_VALID | PTE_UXN)) > > #define pte_valid_user(pte) \ > > ((pte_val(pte) & (PTE_VALID | PTE_USER)) == (PTE_VALID | PTE_USER)) > > Won't pte_valid_user() go wrong for exec-only mappings now? I wondered about the same in November (and reproducing my comment below): https://lore.kernel.org/linux-arm-kernel/20201119182236.GN4376@gaia/ pte_valid_user() checks for both PTE_VALID and PTE_USER. In theory, a !PTE_UXN is also user-accessible but it's only used in gup_pte_range() via pte_access_permitted(). If "access" in the latter means only read/write, we should be ok. If we keep it as is, a comment would be useful. > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > > index 8b5e7e5..47e9fdc 100644 > > --- a/arch/arm64/include/asm/sysreg.h > > +++ b/arch/arm64/include/asm/sysreg.h > > @@ -591,6 +591,7 @@ > > (SCTLR_EL2_RES1 | ENDIAN_SET_EL2) > > > > /* SCTLR_EL1 specific flags. */ > > +#define SCTLR_EL1_EPAN (BIT(57)) > > #define SCTLR_EL1_ATA0 (BIT(42)) > > > > #define SCTLR_EL1_TCF0_SHIFT 38 > > @@ -631,7 +632,7 @@ > > SCTLR_EL1_SED | SCTLR_ELx_I | SCTLR_EL1_DZE | SCTLR_EL1_UCT | \ > > SCTLR_EL1_NTWE | SCTLR_ELx_IESB | SCTLR_EL1_SPAN | SCTLR_ELx_ITFSB | \ > > SCTLR_ELx_ATA | SCTLR_EL1_ATA0 | ENDIAN_SET_EL1 | SCTLR_EL1_UCI | \ > > - SCTLR_EL1_RES1) > > + SCTLR_EL1_EPAN | SCTLR_EL1_RES1) > > Why is this handled differently to normal PAN, where the SCTLR is written in > cpu_enable_pan()? That's how it was done in an early version but I suggested we move it to the default SCTLR bits to save some lines: https://lore.kernel.org/linux-arm-kernel/20201202182303.GC21091@gaia/ With classic PAN, we only enable it if all the CPUs support it. For EPAN that's not necessary since EPAN should depend on PAN being enabled. It's also cached in the TLB, so it's a bit of a hassle with CnP. See this past discussion: https://lore.kernel.org/linux-arm-kernel/X7P+r%2Fl3ewvaf1aV@trantor/ > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > index 3c40da4..c32095f6 100644 > > --- a/arch/arm64/mm/fault.c > > +++ b/arch/arm64/mm/fault.c > > @@ -529,6 +529,9 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr, > > if (faulthandler_disabled() || !mm) > > goto no_context; > > > > + if (cpus_have_const_cap(ARM64_HAS_EPAN)) > > + vm_flags &= ~VM_EXEC; > > This needs a comment, and I think it would be cleaner to rework the vm_flags > initialisation along the lines of: > > unsigned long vm_flags; > unsigned long mm_flags = FAULT_FLAG_DEFAULT; > > if (user_mode(regs)) > mm_flags |= FAULT_FLAG_USER; > > if (is_el0_instruction_abort(esr)) { > vm_flags = VM_EXEC; > mm_flags |= FAULT_FLAG_INSTRUCTION; > } else if (is_write_abort(esr)) { > vm_flags = VM_WRITE; > mm_flags |= FAULT_FLAG_WRITE; > } else { > vm_flags = VM_READ | VM_WRITE; > if (!cpus_have_const_cap(ARM64_HAS_EPAN)) > vm_flags |= VM_EXEC: > } > > (but again, please add a comment to that last part because I still don't > really follow what you're doing) Every time I come across this vm_flags handling I have to spend some minutes to re-understand what it does. So vm_flags tells us what bits we must have in vma->vm_flags for the fault to be benign. But we start with all rwx flags and clear VM_EXEC if EPAN since exec does not imply read, making it more confusing. I think your rewrite is cleaner, maybe with some comment why we add VM_EXEC when !EPAN. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel