Re: [PATCH v10 2/2] arm64: Enable BTI for main executable as well as the interpreter

From: Catalin Marinas <catalin.marinas@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Jeremy Linton <jeremy.linton@arm.com>,
	"H . J . Lu" <hjl.tools@gmail.com>,
	Yu-cheng Yu <yu-cheng.yu@intel.com>,
	Kees Cook <keescook@chromium.org>,
	Eric Biederman <ebiederm@xmission.com>,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	libc-alpha@sourceware.org
Subject: Re: [PATCH v10 2/2] arm64: Enable BTI for main executable as well as the interpreter
Date: Mon, 28 Feb 2022 18:24:07 +0000	[thread overview]
Message-ID: <Yh0TR3l6kWpsg0uu@arm.com> (raw)
In-Reply-To: <20220228130606.1070960-3-broonie@kernel.org>

On Mon, Feb 28, 2022 at 01:06:06PM +0000, Mark Brown wrote:
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 5369e649fa79..82aaf361fa17 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -701,23 +701,49 @@ core_initcall(tagged_addr_init);
>  #endif	/* CONFIG_ARM64_TAGGED_ADDR_ABI */
>  
>  #ifdef CONFIG_BINFMT_ELF
> +static unsigned int bti_main;
> +
>  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;
> -
> -	if (prot & PROT_EXEC)
> +	if ((prot & PROT_EXEC) &&
> +	    (is_interp || !has_interp || bti_main) &&
> +	    (state->flags & arm64_elf_bti_flag(is_interp)))
>  		prot |= PROT_BTI;
>  
>  	return prot;
>  }

TBH, I liked the other series more as we were getting rid of
'has_interp' in patches 3 and 4. Now we keep it around only for the
bti_main case on dynamic executables (i.e. we need to distinguish them
from static). We could still get rid of has_interp if bti_main was
default on and it affected static binaries as well (for consistency, it
wouldn't be a bad idea).

I think the risk of ABI breaking is negligible in a glibc distro since
currently the dynamic loader sets PROT_BTI on the main exe anyway, just
as the kernel does after this series.

Anyway, from a correctness perspective, this patch looks fine to me,
just a preference for the other series:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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