linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Dave Martin <Dave.Martin@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Jeremy Linton <jeremy.linton@arm.com>,
	"H . J . Lu" <hjl.tools@gmail.com>,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	libc-alpha@sourceware.org
Subject: Re: [PATCH v1 2/2] arm64: Enable BTI for main executable as well as the interpreter
Date: Thu, 3 Jun 2021 16:40:35 +0100	[thread overview]
Message-ID: <20210603154034.GH4187@arm.com> (raw)
In-Reply-To: <20210521144621.9306-3-broonie@kernel.org>

On Fri, May 21, 2021 at 03:46:21PM +0100, Mark Brown wrote:
> Currently for dynamically linked ELF executables we only enable BTI for
> the interpreter, expecting the interpreter to do this for the main
> executable. This is a bit inconsistent since we do map main executable and
> is causing issues with systemd's MemoryDenyWriteExecute feature which is
> implemented using a seccomp filter which prevents setting PROT_EXEC on
> already mapped memory and lacks the context to be able to detect that
> memory is already mapped with PROT_EXEC.

It's hard to know whether this is an extensibility fail in the
semantics of mprotect() (and so we were wrong to add PROT_BTI there in
line with my original proposal), or whether this is a case of systemd
doing something that is broken by design (if well-intentioned).  Since
there have been wacky arch-specific mprotect flags around for a fair
while I'd be tempted to argue the latter -- but then I am biased.


Anyway, although I'm a bit queasy about the cause of this patch, the
patch itself looks perfectly reasonable.  If nothing else, it makes
sense as a cleanup or optimisation, so that ld.so doesn't have to do a
bunch of mprotect() calls every time it loads a program.

Do we know how libcs will detect that they don't need to do the
mprotect() calls?  Do we need a detection mechanism at all?

Ignoring certain errors from mprotect() when ld.so is trying to set
PROT_BTI on the main executable's code pages is probably a reasonable,
backwards-compatible compromise here, but it seems a bit wasteful.

> Resolve this by checking the BTI property for the main executable and
> enabling BTI if it is present when doing the initial mapping. This does
> mean that we may get more code with BTI enabled if running on a system
> without BTI support in the dynamic linker, this is expected to be a safe
> configuration and testing seems to confirm that. It also reduces the

Ack, plus IIUC the architecture is designed so that everything works
providing that PROT_BTI is never set on non-BTI-aware code pages.  For
BTI-aware code, the sooner we set PROT_BTI the better I guess.

> flexibility userspace has to disable BTI but it is expected that for cases
> where there are problems which require BTI to be disabled it is more likely
> that it will need to be disabled on a system level.

There's no flexibility impact unless MemoryDenyWriteExecute is in force,
right?

Self-modifying programs (JITs etc.) already can't use that IIUC, so
shouldn't be affected.  That seems the main scenario where people are
likely to be twiddling PROT_{EXEC,WRITE,BTI} on existing pages.

If the main binary is marked as supporting BTI but breaks with
PROT_BTI, then that almost certainly means the toolchain, system
libraries or hardware are broken -- so it would be pointless to have an
elegant workaround.  A big global kill switch seems adequate to me.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/elf.h | 14 ++++++++++----
>  arch/arm64/kernel/process.c  | 18 ++++++------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> index c8678a8c36d5..a6e9032b951a 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -253,7 +253,8 @@ struct arch_elf_state {
>  	int flags;
>  };
>  
> -#define ARM64_ELF_BTI		(1 << 0)
> +#define ARM64_ELF_INTERP_BTI		(1 << 0)
> +#define ARM64_ELF_EXEC_BTI		(1 << 1)
>  
>  #define INIT_ARCH_ELF_STATE {			\
>  	.flags = 0,				\
> @@ -274,9 +275,14 @@ static inline int arch_parse_elf_property(u32 type, const void *data,
>  		if (datasz != sizeof(*p))
>  			return -ENOEXEC;
>  
> -		if (system_supports_bti() && is_interp &&
> -		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
> -			arch->flags |= ARM64_ELF_BTI;
> +		if (system_supports_bti() &&
> +		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
> +			if (is_interp) {

Nit: can we drop the extra curlies?

> +				arch->flags |= ARM64_ELF_INTERP_BTI;
> +			} else {
> +				arch->flags |= ARM64_ELF_EXEC_BTI;
> +			}
> +		}
>  	}
>  
>  	return 0;
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index b4bb67f17a2c..f7fff4a4c99f 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -744,19 +744,13 @@ 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 (prot & PROT_EXEC) {
> +		if (state->flags & ARM64_ELF_INTERP_BTI && is_interp)
> +			prot |= PROT_BTI;
>  
> -	if (!(state->flags & ARM64_ELF_BTI))
> -		return prot;
> -
> -	if (prot & PROT_EXEC)
> -		prot |= PROT_BTI;
> +		if (state->flags & ARM64_ELF_EXEC_BTI && !is_interp)

Merge these ifs together somehow?  I'm happy either way, though.

> +			prot |= PROT_BTI;
> +	}

Since is_interp and has_interp were only needed for this logic in the
first place, I think we can probably drop those, maybe in a subsequent
patch.  Probably better to do it now before too much dust settles on
them.

Again, Cc Yu-cheng Yu if doing that, since it might affect his patches.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

(though if some of the suggested changes are made elsewhere, this will
probably need a minor respin).

Cheers
---Dave

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

  reply	other threads:[~2021-06-03 16:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-21 14:46 [PATCH v1 0/2] arm64: Enable BTI for the " Mark Brown
2021-05-21 14:46 ` [PATCH v1 1/2] elf: Allow architectures to parse properties on the main executable Mark Brown
2021-06-03 15:40   ` Dave Martin
2021-06-03 18:52     ` Mark Brown
2021-05-21 14:46 ` [PATCH v1 2/2] arm64: Enable BTI for main executable as well as the interpreter Mark Brown
2021-06-03 15:40   ` Dave Martin [this message]
2021-06-03 16:51     ` Mark Brown
2021-06-03 18:04       ` Catalin Marinas
2021-06-07 11:25         ` Dave Martin
2021-06-07 18:12           ` Catalin Marinas
2021-06-08 11:33             ` Mark Brown
2021-06-08 15:19               ` Dave Martin
2021-06-08 15:42                 ` Jeremy Linton
2021-06-10 10:33                   ` Dave Martin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210603154034.GH4187@arm.com \
    --to=dave.martin@arm.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=hjl.tools@gmail.com \
    --cc=jeremy.linton@arm.com \
    --cc=libc-alpha@sourceware.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=szabolcs.nagy@arm.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v1 2/2] arm64: Enable BTI for main executable as well as the interpreter' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).