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 E2B16C54791 for ; Wed, 13 Mar 2024 10:48:02 +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=dLLeuJzHiaSJaNbrlrTHUkCcLs6AMByKfXT1ASSMlfE=; b=heDNYrHa84m7yV L2g2YcuoM4UMXWVG7bgGMTALZcrdkqd/vjuwde0yJnXbIujgFkwltwpAXNZoykCd0/j+UXK0wiRXR NtRDXVSF7oAVJaehx/iLB1n6TfY9CTJLQs42na+UdKgdMa/LMWaaIwkgSboKkow2qXQoAjipgMP3r d+R3GmINa9uvGGwxyHuzj9UdG6waUVyA7XVQ4UiaDzxJtrqCFV9X2jiOH/eUQ6iKCXnL3Kwgzzrhv ElCo6B+Vp1akKAuBlE2IS0ngDa7ciiIjcSCIyO0MwuhNteAK3fx2n6Snqa1E0T5obhS3AQ49gzBbr IHeD6AHOMTnuhBqFxVxg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rkM9C-00000009jGy-0GZX; Wed, 13 Mar 2024 10:47:46 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rkM98-00000009jGO-2X9d for linux-arm-kernel@lists.infradead.org; Wed, 13 Mar 2024 10:47:44 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id BF0D561345; Wed, 13 Mar 2024 10:47:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1D8CC433C7; Wed, 13 Mar 2024 10:47:38 +0000 (UTC) Date: Wed, 13 Mar 2024 10:47:36 +0000 From: Catalin Marinas To: Ard Biesheuvel Cc: linux-arm-kernel@lists.infradead.org, Will Deacon , Marc Zyngier , Mark Rutland , Ryan Roberts , Anshuman Khandual , Kees Cook , Joey Gouly Subject: Re: [PATCH v8 42/43] mm: add arch hook to validate mmap() prot flags Message-ID: References: <20240214122845.2033971-45-ardb+git@google.com> <20240214122845.2033971-87-ardb+git@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240313_034742_788102_3EDBCF2C X-CRM114-Status: GOOD ( 41.88 ) 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 On Wed, Mar 13, 2024 at 12:23:24AM +0100, Ard Biesheuvel wrote: > On Tue, 12 Mar 2024 at 20:53, Catalin Marinas wrote: > > On Wed, Feb 14, 2024 at 01:29:28PM +0100, Ard Biesheuvel wrote: > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > index d89770eaab6b..977a8c3fd9f5 100644 > > > --- a/mm/mmap.c > > > +++ b/mm/mmap.c > > > @@ -1229,6 +1229,9 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > > > if (!(file && path_noexec(&file->f_path))) > > > prot |= PROT_EXEC; > > > > > > + if (!arch_validate_mmap_prot(prot, addr)) > > > + return -EACCES; > > > + > > > /* force arch specific MAP_FIXED handling in get_unmapped_area */ > > > if (flags & MAP_FIXED_NOREPLACE) > > > flags |= MAP_FIXED; > > > > While writing the pull request for Linus (and looking to justify this > > change), I realised that we already have arch_validate_flags() that can > > do a similar check but on VM_* flags instead of PROT_* (we use it for > > VM_MTE checks). What was the reason for adding a new hook? [...] > > The only > > difference is that here it returns -EACCESS while on > > arch_validate_flags() failure it would return -EINVAL. It probably makes > > more to return -EACCESS as it matches map_deny_write_exec() but with > > your patches we are inconsistent between mmap() and mprotect() errors > > (the latter is still -EINVAL). > > Yes. Looking at it now, it would be better to add a single arch hook > to map_deny_write_exec(), and use that instead. This would work and matches the API better. Another way to look at the arm64 WXN feature is to avoid bothering with with the checks knowing that the hardware enforces XN when a permission is W. So it can be seen as a choice irrespective of the user PROT_EXEC|PROT_WRITE permission. But it's still an ABI change, so I guess better to be upfront with the user and reject such mmap()/mprotect() permission combinations. However, I've been looking through specs and realised that SCTLR_ELx.WXN is RES0 when the permission indirection is enabled (FEAT_PIE from the 2022 specs, hopefully you have access to it). And while apparently WXN gets better as it allows separate EL0/EL1 controls, it seems to only apply when the base permission is RWX and the XN is toggled based on the overlay permission (pkeys which Joey is working on). So it looks like what the architects had in mind is optimising RW/RX switching via overlays (no syscalls) but keeping the base permission RWX. The traditional WXN hardening via SCTLR_EL1 disappeared. (adding Joey to the thread, he contributed the PIE support) > > It also got me thinking on whether we could use this as a hardened > > version of the MDWE feature instead a CPU feature (though we'd end up > > context-switching this SCTLR_EL1 bit). I think your patches have been > > around before the MDWE feature was added to the kernel. > > Indeed. > > MDWE looks like a good match in principle, but combining the two seems tricky: > - EL1 is going to flip between WXN protection on and off depending on > which setting it is using for EL0; > - context switching SCTLR_EL1.WXN may become costly in terms of TLB > maintenance, unless we cheat and ignore the kernel mappings (which > should work as expected regardless of the value of SCTLR_EL1.WXN); > > If we can find a reasonable strategy to manage the TLB maintenance > that does not leave EL1 behind, I'm happy to explore this further. But > perhaps WXN should simply be treated as MDWE always-on for all > processes. Ah, I did not realise this bit can be cached in the TLB. So this doesn't really work (and the Arm ARM is also vague about whether it's cached per ASID or not). > > Sorry for not catching this early. > > No worries - it's more likely to be useful if we get this right. Given that with PIE this feature no longer works, I'll revert these two patches for now. We should revisit it in combination with PIE and POE (overlays) but it does look like the semantics are slightly different (unless I misread the specs). If we want a global MDWE=on on the command line, this can be generic. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel