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

[Dave Martin <Dave.Martin@arm.com>]

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

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