Message ID | 20210205173837.39315-1-broonie@kernel.org |
---|---|
State | Not applicable |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 700BE39F4435; Fri, 5 Feb 2021 17:39:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 700BE39F4435 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1612546784; bh=L2UHFCa9Zc51PE0WWey49NT4Bzo8RBLP6MzntQCF0a8=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=rlFFlArqG9iaLogb1lIAz5yjYu0vA9s8wOdcCDMV7nyqZV8q3J4uK/p63wYl8SB0j +IQBUiAGrJNHj6z/JwJFcmK3qs5eyvxYAcwO7OuRZO0aPGOItRPxl8cFp/SzLo2nGj LpKsyMONyzmlG3sM/23S4yUoF42j21ArIVh4mW/Y= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by sourceware.org (Postfix) with ESMTPS id 297F3385802B for <libc-alpha@sourceware.org>; Fri, 5 Feb 2021 17:39:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 297F3385802B Received: by mail.kernel.org (Postfix) with ESMTPSA id B6DE260C3F; Fri, 5 Feb 2021 17:39:39 +0000 (UTC) To: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org> Subject: [PATCH] arm64: bti: Set PROT_BTI on all BTI executables mapped by the kernel Date: Fri, 5 Feb 2021 17:38:37 +0000 Message-Id: <20210205173837.39315-1-broonie@kernel.org> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 X-Patch-Hashes: v=1; h=sha256; i=7zN43aXdXMdmfaLj4FX8gMm7a76OOvJsogge4/nn8Y4=; m=dy0D/ccU8fG3zSNioE+eBi2SH0YOdNWPkq8kfTP7sq8=; p=fDryMjcYCFaY94nuNd8JVY/L7W1xGukVcWRmSZ1mHXY=; g=0f8818e661955edd30fd55277d55e6b5dbe31f48 X-Patch-Sig: m=pgp; i=broonie@kernel.org; s=0xC3F436CA30F5D8EB; b=iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmAdgP4ACgkQJNaLcl1Uh9AN/gf+N1V Uu7Zv9yx+E+3Fn5F8dUqn4p4hjv2ILNoQzG1dwNQljW8z/HJyyR8Gm+zsHfWyBJ3D5kAIxXAQzjq4 GJ7dLF1uWNcsRzT/nZQGqTMBI4jOhb4Rb0RR4hH4zpAGZ5OWasC2Dd+u+6nVzyRs2Nv+31w/LQoBr ggZGk18HL66O+jaOd14DGLjVWpbbjKTytnGiq3T66p/JEm9IN5lRNjHWeEjshkSsy1auIxZfjLzPZ GXXmR7U865dKTjdT7y5jG5087vu5HUXQNn1vNBPREm06p+k78Qby+4s3baWUnHmmbgaZiewJjqm6v 4RqYRxexAbQNyWZr7/EI/nhPpVgQwWQ== Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Mark Brown via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Mark Brown <broonie@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com>, libc-alpha@sourceware.org, Kees Cook <keescook@chromium.org>, Jeremy Linton <jeremy.linton@arm.com>, Mark Brown <broonie@kernel.org>, Dave Martin <dave.martin@arm.com>, linux-arm-kernel@lists.infradead.org Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
arm64: bti: Set PROT_BTI on all BTI executables mapped by the kernel
|
|
Commit Message
Mark Brown
Feb. 5, 2021, 5:38 p.m. UTC
Currently for dynamically linked executables the kernel only enables
PROT_BTI for the interpreter, the interpreter is responsible for
enabling it for everything else including the main executable.
Unfortunately this interacts poorly with systemd's
MemoryDenyWriteExecute feature which uses a seccomp filter to prevent
setting PROT_EXEC on already mapped memory via mprotect(), it lacks the
context to detect that PROT_EXEC is already set and so refuses to allow
the mprotect() on the main executable which the kernel has already
mapped.
Since we don't want to force users to choose between having MDWX and BTI
as these are othogonal features have the kernel enable PROT_BTI for all
the ELF objects it loads, not just the dynamic linker. This means that
if there is a problem with BTI it will be harder to disable at the
executable level but we currently have no conditional support for this
in any libc anyway so that would be new development. Ideally we would
have interfaces that allowed us to more clearly specify what is enabled
and disabled by a given syscall but this would be a far more difficult
change to deploy.
Reported-by: Jeremy Linton <jeremy.linton@arm.com>
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Szabolcs Nagy <szabolcs.nagy@arm.com>
Cc: Dave Martin <dave.martin@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: libc-alpha@sourceware.org
---
This solution was proposed by Catalin, I'm just writing it up into a
patch since it looks to be what we've converged on as the most practical
solution and but things seemed to have stalled out.
arch/arm64/kernel/process.c | 8 --------
1 file changed, 8 deletions(-)
Comments
On Fri, Feb 05, 2021 at 05:38:37PM +0000, Mark Brown wrote: > Currently for dynamically linked executables the kernel only enables > PROT_BTI for the interpreter, the interpreter is responsible for > enabling it for everything else including the main executable. > Unfortunately this interacts poorly with systemd's > MemoryDenyWriteExecute feature which uses a seccomp filter to prevent > setting PROT_EXEC on already mapped memory via mprotect(), it lacks the > context to detect that PROT_EXEC is already set and so refuses to allow > the mprotect() on the main executable which the kernel has already > mapped. > > Since we don't want to force users to choose between having MDWX and BTI > as these are othogonal features have the kernel enable PROT_BTI for all > the ELF objects it loads, not just the dynamic linker. This means that > if there is a problem with BTI it will be harder to disable at the > executable level but we currently have no conditional support for this > in any libc anyway so that would be new development. Ideally we would > have interfaces that allowed us to more clearly specify what is enabled > and disabled by a given syscall but this would be a far more difficult > change to deploy. > > Reported-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Mark Brown <broonie@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Szabolcs Nagy <szabolcs.nagy@arm.com> > Cc: Dave Martin <dave.martin@arm.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: libc-alpha@sourceware.org Thanks Mark for putting the patch together. You may want to add a reference to some of the discussions around the ABI, one of them: Link: https://lore.kernel.org/r/20201207200338.GB24625@arm.com/ (so we can keep Szabolcs accountable if something breaks ;)) For this patch: Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will is ok. It would give us some time to revert.
On Fri, Feb 05, 2021 at 05:51:29PM +0000, Catalin Marinas wrote: > Thanks Mark for putting the patch together. You may want to add a > reference to some of the discussions around the ABI, one of them: > Link: https://lore.kernel.org/r/20201207200338.GB24625@arm.com/ > (so we can keep Szabolcs accountable if something breaks ;)) Done locally. > For this patch: > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will > is ok. It would give us some time to revert. Yes, I was expecting v5.12 at the earliest.
On Fri, Feb 05, 2021 at 05:51:29PM +0000, Catalin Marinas wrote: > On Fri, Feb 05, 2021 at 05:38:37PM +0000, Mark Brown wrote: > > Currently for dynamically linked executables the kernel only enables > > PROT_BTI for the interpreter, the interpreter is responsible for > > enabling it for everything else including the main executable. > > Unfortunately this interacts poorly with systemd's > > MemoryDenyWriteExecute feature which uses a seccomp filter to prevent > > setting PROT_EXEC on already mapped memory via mprotect(), it lacks the > > context to detect that PROT_EXEC is already set and so refuses to allow > > the mprotect() on the main executable which the kernel has already > > mapped. > > > > Since we don't want to force users to choose between having MDWX and BTI > > as these are othogonal features have the kernel enable PROT_BTI for all > > the ELF objects it loads, not just the dynamic linker. This means that > > if there is a problem with BTI it will be harder to disable at the > > executable level but we currently have no conditional support for this > > in any libc anyway so that would be new development. Ideally we would > > have interfaces that allowed us to more clearly specify what is enabled > > and disabled by a given syscall but this would be a far more difficult > > change to deploy. > > > > Reported-by: Jeremy Linton <jeremy.linton@arm.com> > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > > Signed-off-by: Mark Brown <broonie@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Szabolcs Nagy <szabolcs.nagy@arm.com> > > Cc: Dave Martin <dave.martin@arm.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: libc-alpha@sourceware.org > > Thanks Mark for putting the patch together. You may want to add a > reference to some of the discussions around the ABI, one of them: > > Link: https://lore.kernel.org/r/20201207200338.GB24625@arm.com/ > > (so we can keep Szabolcs accountable if something breaks ;)) > > For this patch: > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > > I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will > is ok. It would give us some time to revert. I'd like an Ack from Szabolcs before we queue this. Will
The 02/08/2021 12:44, Will Deacon wrote: > On Fri, Feb 05, 2021 at 05:51:29PM +0000, Catalin Marinas wrote: > > On Fri, Feb 05, 2021 at 05:38:37PM +0000, Mark Brown wrote: > > > Currently for dynamically linked executables the kernel only enables > > > PROT_BTI for the interpreter, the interpreter is responsible for > > > enabling it for everything else including the main executable. > > > Unfortunately this interacts poorly with systemd's > > > MemoryDenyWriteExecute feature which uses a seccomp filter to prevent > > > setting PROT_EXEC on already mapped memory via mprotect(), it lacks the > > > context to detect that PROT_EXEC is already set and so refuses to allow > > > the mprotect() on the main executable which the kernel has already > > > mapped. > > > > > > Since we don't want to force users to choose between having MDWX and BTI > > > as these are othogonal features have the kernel enable PROT_BTI for all > > > the ELF objects it loads, not just the dynamic linker. This means that > > > if there is a problem with BTI it will be harder to disable at the > > > executable level but we currently have no conditional support for this > > > in any libc anyway so that would be new development. Ideally we would > > > have interfaces that allowed us to more clearly specify what is enabled > > > and disabled by a given syscall but this would be a far more difficult > > > change to deploy. > > > > > > Reported-by: Jeremy Linton <jeremy.linton@arm.com> > > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > > > Signed-off-by: Mark Brown <broonie@kernel.org> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: Szabolcs Nagy <szabolcs.nagy@arm.com> > > > Cc: Dave Martin <dave.martin@arm.com> > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: libc-alpha@sourceware.org > > > > Thanks Mark for putting the patch together. You may want to add a > > reference to some of the discussions around the ABI, one of them: > > > > Link: https://lore.kernel.org/r/20201207200338.GB24625@arm.com/ > > > > (so we can keep Szabolcs accountable if something breaks ;)) > > > > For this patch: > > > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > > > > I wouldn't merge it as a fix yet but I'm ok with getting in 5.12 if Will > > is ok. It would give us some time to revert. > > I'd like an Ack from Szabolcs before we queue this. > > Will i'm ok with this in principle, but i will rerun the glibc tests over night to be sure.
On Fri, Feb 05, 2021 at 05:38:37PM +0000, Mark Brown via Libc-alpha wrote: > Currently for dynamically linked executables the kernel only enables > PROT_BTI for the interpreter, the interpreter is responsible for > enabling it for everything else including the main executable. > Unfortunately this interacts poorly with systemd's > MemoryDenyWriteExecute feature which uses a seccomp filter to prevent > setting PROT_EXEC on already mapped memory via mprotect(), it lacks the > context to detect that PROT_EXEC is already set and so refuses to allow > the mprotect() on the main executable which the kernel has already > mapped. > > Since we don't want to force users to choose between having MDWX and BTI > as these are othogonal features have the kernel enable PROT_BTI for all > the ELF objects it loads, not just the dynamic linker. This means that > if there is a problem with BTI it will be harder to disable at the > executable level but we currently have no conditional support for this > in any libc anyway so that would be new development. Ideally we would > have interfaces that allowed us to more clearly specify what is enabled > and disabled by a given syscall but this would be a far more difficult > change to deploy. > > Reported-by: Jeremy Linton <jeremy.linton@arm.com> > Suggested-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Mark Brown <broonie@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Szabolcs Nagy <szabolcs.nagy@arm.com> > Cc: Dave Martin <dave.martin@arm.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: libc-alpha@sourceware.org > --- > > This solution was proposed by Catalin, I'm just writing it up into a > patch since it looks to be what we've converged on as the most practical > solution and but things seemed to have stalled out. > > arch/arm64/kernel/process.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 71c8265b9139..0967f9e1f9fd 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -717,14 +717,6 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void) > int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state, > bool has_interp, bool is_interp) > { > - /* > - * For dynamically linked executables the interpreter is > - * responsible for setting PROT_BTI on everything except > - * itself. > - */ > - if (is_interp != has_interp) > - return prot; > - Reviewed-by: Dave Martin <Dave.Martin@arm.com> > if (!(state->flags & ARM64_ELF_BTI)) > return prot; The original idea was to interfere with userspace as little as possible, and leave this until/unless there was a clear need for it and a clear understanding that it wouldn't break anything. Looks like we have both of those now -- I'll leave it to Szabolcs to confirm the userspace view of this. Question: will this change prevent BTI executables from working under a non-BTI-aware ld.so? And do we care? (I think "probably not" for both, but I'd be interested in others' views.) Cheers ---Dave
On Mon, Feb 08, 2021 at 02:53:16PM +0000, Dave Martin wrote: > Question: will this change prevent BTI executables from working under a > non-BTI-aware ld.so? And do we care? (I think "probably not" for both, > but I'd be interested in others' views.) Yes, I'd expect that we'd end up allowing a BTIed main executable without a BTI ld.so. We already have the case where we will enable BTI for the vDSO even if nothing outside the kernel supports BTI and BTI is intended to interwork happily so I'd be surprised if there were problems.
The 02/08/2021 14:13, Szabolcs Nagy via Libc-alpha wrote: > The 02/08/2021 12:44, Will Deacon wrote: > > I'd like an Ack from Szabolcs before we queue this. > > i'm ok with this in principle, but i will rerun > the glibc tests over night to be sure. the patch applied cleanly on top of arm64 for-next/core but it does not work as i expected: executables that do not have the bti property note set seems to get bti guarded by the kernel. at least i see crash in _start when the dynamic linker (which has bti marking) transfers control to it and the start code has no bti c. (according to strace the dynamic linker did not remap/mprotect the main exe with bti so i assume this is the kernel's doing) can somebody verify that the notes are checked on the executable too and not just on ld.so?
The 02/08/2021 15:06, Mark Brown via Libc-alpha wrote: > On Mon, Feb 08, 2021 at 02:53:16PM +0000, Dave Martin wrote: > > > Question: will this change prevent BTI executables from working under a > > non-BTI-aware ld.so? And do we care? (I think "probably not" for both, > > but I'd be interested in others' views.) > > Yes, I'd expect that we'd end up allowing a BTIed main executable > without a BTI ld.so. We already have the case where we will enable BTI > for the vDSO even if nothing outside the kernel supports BTI and BTI is > intended to interwork happily so I'd be surprised if there were > problems. note that this is unlikely to happen (as opposed to the other way: bti marked ld.so & non-bti exe) because if the libc is not built with bti then the start code (crt1.o) has no bti marking either so the exe will not get bti marked.
On Mon, Feb 08, 2021 at 04:47:45PM +0000, Szabolcs Nagy via Libc-alpha wrote: > The 02/08/2021 14:13, Szabolcs Nagy via Libc-alpha wrote: > > The 02/08/2021 12:44, Will Deacon wrote: > > > I'd like an Ack from Szabolcs before we queue this. > > > > i'm ok with this in principle, but i will rerun > > the glibc tests over night to be sure. > > > the patch applied cleanly on top of arm64 for-next/core > > but it does not work as i expected: > > executables that do not have the bti property note > set seems to get bti guarded by the kernel. > > at least i see crash in _start when the dynamic > linker (which has bti marking) transfers control > to it and the start code has no bti c. > > (according to strace the dynamic linker did not > remap/mprotect the main exe with bti so i assume > this is the kernel's doing) > > can somebody verify that the notes are checked > on the executable too and not just on ld.so? Reviewed-by bites the dust... Aha, looking at the ELF code in the kernel, it looks like some extra refactoring is needed. We do the heavy lifting only for the image containing the userspace entry point -- i.e., ld.so in the dynamically linked case. This includes the ELF property handling. When ld.so is present, the main executable is just data so we map it in but don't do a whole lot else with it: static int load_elf_binary(struct linux_binprm *bprm) { /* ... */ retval = parse_elf_properties(interpreter ?: bprm->file, elf_property_phdata, &arch_state); The way I originally integrated this therefore just tracks the BTI-ness (and/or equivalently how to mutate PROT_EXEC) once. Looks like we need to do that independently for ld.so and for the executable instead. We could simplify things by treating it as an error if the executable and ld.so have different BTI properties, but that seems a bit of an own goal, since it breaks foreseeable backwards compatibility / hybrid use cases. Cheers ---Dave
On Mon, Feb 08, 2021 at 05:40:29PM +0000, Dave P Martin wrote: > On Mon, Feb 08, 2021 at 04:47:45PM +0000, Szabolcs Nagy via Libc-alpha wrote: > > The 02/08/2021 14:13, Szabolcs Nagy via Libc-alpha wrote: > > > The 02/08/2021 12:44, Will Deacon wrote: > > > > I'd like an Ack from Szabolcs before we queue this. > > > > > > i'm ok with this in principle, but i will rerun > > > the glibc tests over night to be sure. > > > > > > the patch applied cleanly on top of arm64 for-next/core > > > > but it does not work as i expected: > > > > executables that do not have the bti property note > > set seems to get bti guarded by the kernel. > > > > at least i see crash in _start when the dynamic > > linker (which has bti marking) transfers control > > to it and the start code has no bti c. > > > > (according to strace the dynamic linker did not > > remap/mprotect the main exe with bti so i assume > > this is the kernel's doing) > > > > can somebody verify that the notes are checked > > on the executable too and not just on ld.so? Thanks Szabolcs for testing. > Reviewed-by bites the dust... > > Aha, looking at the ELF code in the kernel, it looks like some extra > refactoring is needed. > > We do the heavy lifting only for the image containing the userspace > entry point -- i.e., ld.so in the dynamically linked case. This > includes the ELF property handling. When ld.so is present, the main > executable is just data so we map it in but don't do a whole lot else > with it: > > static int load_elf_binary(struct linux_binprm *bprm) > { > /* ... */ > > retval = parse_elf_properties(interpreter ?: bprm->file, > elf_property_phdata, &arch_state); > > The way I originally integrated this therefore just tracks the BTI-ness > (and/or equivalently how to mutate PROT_EXEC) once. Looks like we need > to do that independently for ld.so and for the executable instead. > > We could simplify things by treating it as an error if the executable > and ld.so have different BTI properties, but that seems a bit of an own > goal, since it breaks foreseeable backwards compatibility / hybrid use > cases. Ah, I haven't thought of these cases either. In theory, if the dynamic loader hasn't been compiled with -mbranch-protection=bti, it does not mean it won't be able to use PROT_BTI on the mapped executable/libraries. The same goes for BTI-compiled loader with a non-BTI executable, it should be allowed. So I think decoupling the two makes sense but the patch is no longer as simple as the ELF loader now needs to parse the main executable. If that's doable and the code looks sane, I'd go with it. Otherwise, we scrap the idea of the kernel setting PROT_BTI on the main executable.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 71c8265b9139..0967f9e1f9fd 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -717,14 +717,6 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void) int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state, bool has_interp, bool is_interp) { - /* - * For dynamically linked executables the interpreter is - * responsible for setting PROT_BTI on everything except - * itself. - */ - if (is_interp != has_interp) - return prot; - if (!(state->flags & ARM64_ELF_BTI)) return prot;