Re: [PATCH] arm64: bti: Set PROT_BTI on all BTI executables mapped by the kernel

From: Catalin Marinas <catalin.marinas@arm.com>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	libc-alpha@sourceware.org, Kees Cook <keescook@chromium.org>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Jeremy Linton <jeremy.linton@arm.com>,
	Mark Brown <broonie@kernel.org>, Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arm64: bti: Set PROT_BTI on all BTI executables mapped by the kernel
Date: Mon, 8 Feb 2021 18:49:03 +0000	[thread overview]
Message-ID: <20210208184902.GC12694@gaia> (raw)
In-Reply-To: <20210208174028.GG21837@arm.com>

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.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel