From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by sourceware.org (Postfix) with ESMTPS id 373B93857C5D for ; Fri, 7 Jan 2022 12:01:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 373B93857C5D Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id E8B7CB8259E; Fri, 7 Jan 2022 12:01:23 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E6DACC36AE0; Fri, 7 Jan 2022 12:01:20 +0000 (UTC) Date: Fri, 7 Jan 2022 12:01:17 +0000 From: Catalin Marinas To: Mark Brown Cc: Jeremy Linton , Szabolcs Nagy , Will Deacon , "H . J . Lu" , Yu-cheng Yu , linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, libc-alpha@sourceware.org, Mark Rutland Subject: Re: [PATCH v7 0/4] arm64: Enable BTI for the executable as well as the interpreter Message-ID: References: <20211115152714.3205552-1-broonie@kernel.org> <20211209111048.GM3294453@arm.com> <101d8e84-7429-bbf1-0271-5436eca0eea2@arm.com> <8550afd2-268d-a25f-88fd-0dd0b184ca23@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Jan 2022 12:01:26 -0000 On Thu, Jan 06, 2022 at 07:07:46PM +0000, Mark Brown wrote: > On Thu, Jan 06, 2022 at 06:13:37PM +0000, Catalin Marinas wrote: > > On Thu, Jan 06, 2022 at 10:09:35AM -0600, Jeremy Linton wrote: > > > This should only change the behavior for for binaries which conform to the > > > new ABI containing the BTI note. So outside of the tiny window of things > > > built with BTI, but run on !BTI hardware or older kernel+glibc, this > > > shouldn't be a problem. (Unless i'm missing something) Put another way, now > > > is the time to make a change, before there is a legacy BTI ecosystem we have > > > to deal with. > > > The concern is that the loader may decide in the future to not enable > > (or turn off) BTI for some reason (e.g. mixed libraries, old glibc on > > BTI hardware). If we force BTI on the main executable, we'd take this > > option away. Note also that it's not only glibc here, there are other > > loaders. > > Neither of those examples should be concerns - BTI is per page so you > can mix BTI and non-BTI freely in a process (as will happen now for the > case where the dynamic loader is built for BTI but the main executable > is not, and the dynamic loader should do if there's a mix of BTI and > non-BTI libraries). The main case where there might be a change would > be the case where there's individual excutables with incorrect BTI > annotations which are run under this seccomp MWDE, then the dynamic > loader might have support for disabling BTI based on some configuration > but wouldn't be able to due to the MWDE. > > Note also that we're only taking the option of disabiling BTI away in > the case where there's something like this seccomp filter disabling > permission changes. Thanks for the clarification. I think we can look at this from two angles: 1. Ignoring MDWE, should whoever does the original mmap() also honour PROT_BTI? We do this for static binaries but, for consistency, should we extend it to dynamic executable? 2. A 'simple' fix to allow MDWE together with BTI. Regarding (1), I don't remember whether we decided to do it this way because it was more complicated to handle it in the kernel (like the 4 more patches in this series) or because we wanted to leave the option to the dynamic loader. It would be good to clarify this and we may have a small window, as Jeremy said, where changing the ABI won't cause problems (well, hopefully, there's still a risk). If we only want (2), I just think we are approaching it wrongly. Looking at mprotect(), this systemd feature is not even MDWE but rather "deny mprotect execute" disregarding the previous attributes. If we want this, something like below would do (conditional on some personality flag or a prctl): diff --git a/mm/mprotect.c b/mm/mprotect.c index e552f5e0ccbd..4262e6f1c14e 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -616,6 +616,11 @@ static int do_mprotect_pkey(unsigned long start, size_t len, goto out; } + if ((newflags & VM_EXEC) && !(vma->vm_flags & VM_EXEC)) { + error = -EACCES; + goto out; + } + /* Allow architectures to sanity-check the new flags */ if (!arch_validate_flags(newflags)) { error = -EINVAL; But we could also do a proper MDWE covering both mmap() and mprotect() and keep track on whether a vma was previously writable. A personality flag seems like a better option as we have READ_IMPLIES_EXEC, we could add a DENY_WRITE_EXEC. -- Catalin