arm64: Enable BTI for the executable as well as the interpreter

Currently the ELF code only attempts to parse properties on the image that will start execution, either the interpreter or for statically linked executables the main executable. The expectation is that any property handling for the main executable will be done by the interpreter. This is a bit inconsistent since we do map the executable and is causing problems for the arm64 BTI support when used in conjunction with systemd's use of seccomp to implement MemoryDenyWriteExecute which stops the dynamic linker adjusting the permissions of executable segments.

Allow architectures to handle properties for both the dynamic linker and main executable, adjusting arch_parse_elf_properties() to have an is_interp flag as with arch_elf_adjust_prot() and calling it for both the main executable and any intepreter.

Signed-off-by Mark Brown <[email protected]>

fs/binfmt_elf.c +13 -6

index 187b3f2b9202..253ca9969345 100644
@@ -716,8 +716,9 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
  */
 
 static int parse_elf_property(const char *data, size_t *off, size_t datasz,
-			      struct arch_elf_state *arch,
-			      bool have_prev_type, u32 *prev_type)
+			      struct arch_elf_state *arch, bool has_interp,
+			      bool is_interp, bool have_prev_type,
+			      u32 *prev_type)
 {
 	size_t o, step;
 	const struct gnu_property *pr;

@@ -751,7 +752,8 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz,
 	*prev_type = pr->pr_type;
 
 	ret = arch_parse_elf_property(pr->pr_type, data + o,
-				      pr->pr_datasz, ELF_COMPAT, arch);
+				      pr->pr_datasz, ELF_COMPAT,
+				      has_interp, is_interp, arch);
 	if (ret)
 		return ret;
 

@@ -764,6 +766,7 @@ static int parse_elf_property(const char *data, size_t *off, size_t datasz,
 #define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME))
 
 static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
+				bool has_interp, bool is_interp,
 				struct arch_elf_state *arch)
 {
 	union {

@@ -813,7 +816,8 @@ static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
 	have_prev_type = false;
 	do {
 		ret = parse_elf_property(note.data, &off, datasz, arch,
-					 have_prev_type, &prev_type);
+					 has_interp, is_interp, have_prev_type,
+					 &prev_type);
 		have_prev_type = true;
 	} while (!ret);
 

@@ -828,6 +832,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
 	struct elf_phdr *elf_property_phdata = NULL;
+	struct elf_phdr *interp_elf_property_phdata = NULL;
 	unsigned long elf_bss, elf_brk;
 	int bss_prot = 0;
 	int retval, i;

@@ -963,12 +968,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			goto out_free_dentry;
 
 		/* Pass PT_LOPROC..PT_HIPROC headers to arch code */
-		elf_property_phdata = NULL;
 		elf_ppnt = interp_elf_phdata;
 		for (i = 0; i < interp_elf_ex->e_phnum; i++, elf_ppnt++)
 			switch (elf_ppnt->p_type) {
 			case PT_GNU_PROPERTY:
-				elf_property_phdata = elf_ppnt;
+				interp_elf_property_phdata = elf_ppnt;
 				break;
 
 			case PT_LOPROC ... PT_HIPROC:

@@ -979,10 +983,17 @@ static int load_elf_binary(struct linux_binprm *bprm)
 					goto out_free_dentry;
 				break;
 			}
+
+		retval = parse_elf_properties(interpreter,
+					      interp_elf_property_phdata,

It's a little annoying that we add has_interp all over the place only to remove it again later, but I guess that may be the simplest way to keep things bisectable while moving logic around. If so, I don't have a strong opinion on it.

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.

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 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.

Signed-off-by Mark Brown <[email protected]>

Reviewed-by Dave Martin <[email protected]>

---
 arch/arm64/include/asm/elf.h | 14 ++++++++++----
 arch/arm64/kernel/process.c  | 18 ++++++------------
 2 files changed, 16 insertions(+), 16 deletions(-)

arch/arm64/include/asm/elf.h +2 -1

index a488a1329b16..9f86dbce2680 100644
@@ -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;
 

arch/arm64/include/asm/elf.h +1 -0

+		}
 	}
 
 	return 0;

arch/arm64/kernel/process.c +2 -7

index b4bb67f17a2c..f7fff4a4c99f 100644
@@ -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)

arch/arm64/include/asm/elf.h static inline int arch_parse_elf_property(u32 type, const void *data, · line 284

Quoted 10 lines · Mark Brown

arch/arm64/include/asm/elf.h +7 -3

-		if (system_supports_bti() && has_interp == 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) {
+				arch->flags |= ARM64_ELF_INTERP_BTI;
+			} else {
+				arch->flags |= ARM64_ELF_EXEC_BTI;
+			}

Nit: surplus curlies? (coding-style.rst does actually say to drop them when all branches of an if are single-statement one-liners -- I had presumed I was just being pedantic...)

arch/arm64/kernel/process.c asmlinkage void __sched arm64_preempt_schedule_irq(void) · line 753

Quoted 5 lines

arch/arm64/kernel/process.c +3 -2

-	if (prot & PROT_EXEC)
-		prot |= PROT_BTI;
+		if (state->flags & ARM64_ELF_EXEC_BTI && !is_interp)
+			prot |= PROT_BTI;
+	}

Is it worth adding () around the bitwise-& expressions? I'm always a little uneasy about the operator precedence of binary &, although without looking it up I think you're correct.

Feel free to adopt if this appeals to you, otherwise I'm also fine with your version.)

Quoted 16 lines · Dave Martin

arch/arm64/include/asm/elf.h static inline int arch_parse_elf_property(u32 type, const void *data, · line 284

Quoted 10 lines · Mark Brown

arch/arm64/include/asm/elf.h +7 -3

-		if (system_supports_bti() && has_interp == 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) {
+				arch->flags |= ARM64_ELF_INTERP_BTI;
+			} else {
+				arch->flags |= ARM64_ELF_EXEC_BTI;
+			}

Nit: surplus curlies? (coding-style.rst does actually say to drop them when all branches of an if are single-statement one-liners -- I had presumed I was just being pedantic...)

I really think this hurts readability with the nested if inside another if with a multi-line condition.

Quoted 9 lines

arch/arm64/kernel/process.c asmlinkage void __sched arm64_preempt_schedule_irq(void) · line 753

Quoted 5 lines

arch/arm64/kernel/process.c +3 -2

-	if (prot & PROT_EXEC)
-		prot |= PROT_BTI;
+		if (state->flags & ARM64_ELF_EXEC_BTI && !is_interp)
+			prot |= PROT_BTI;
+	}

Is it worth adding () around the bitwise-& expressions? I'm always a little uneasy about the operator precedence of binary &, although without looking it up I think you're correct.

Sure. I'm fairly sure the compiler would've complained about this case if it were ambiguous, I'm vaguely surprised it didn't already.

Quoted 2 lines

Feel free to adopt if this appeals to you, otherwise I'm also fine with your version.)

I'll see what I think when I get back to looking at this properly.

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

Signed-off-by Mark Brown <[email protected]>

---
 arch/arm64/kernel/process.c | 2 +-
 fs/binfmt_elf.c             | 2 +-
 include/linux/elf.h         | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

arch/arm64/kernel/process.c +1 -1

index f7fff4a4c99f..e51c4aa7e048 100644
@@ -742,7 +742,7 @@ asmlinkage void __sched arm64_preempt_schedule_irq(void)
 
 #ifdef CONFIG_BINFMT_ELF
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
-			 bool has_interp, bool is_interp)
+			 bool is_interp)
 {
 	if (prot & PROT_EXEC) {
 		if (state->flags & ARM64_ELF_INTERP_BTI && is_interp)

fs/binfmt_elf.c +1 -1

index 253ca9969345..1aba4e50e651 100644
@@ -580,7 +580,7 @@ static inline int make_prot(u32 p_flags, struct arch_elf_state *arch_state,
 	if (p_flags & PF_X)
 		prot |= PROT_EXEC;
 
-	return arch_elf_adjust_prot(prot, arch_state, has_interp, is_interp);
+	return arch_elf_adjust_prot(prot, arch_state, is_interp);
 }
 
 /* This is much more generalized than the library routine read function,

include/linux/elf.h +0 -0

index 1c45ecf29147..d8392531899d 100644

Quoted 5 lines · Mark Brown

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

So far so good, but can we also drop the has_interp argument from arch_parse_elf_properties()?

Cross-check with Yu-Cheng Yu's series, but I don't see this being used any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally to each other now, I don't think we should need a has_interp argument to pass knowledge between the interpreter and executable handling phases here.

Quoted 10 lines · Dave Martin

Quoted 5 lines · Mark Brown

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

So far so good, but can we also drop the has_interp argument from arch_parse_elf_properties()?

Quoted 8 lines

Cross-check with Yu-Cheng Yu's series, but I don't see this being used any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally to each other now, I don't think we should need a has_interp argument to pass knowledge between the interpreter and executable handling phases here.

For CET, arch_parse_elf_property() needs to know has_interp and is_interp. Like the following, on top of your patches:

arch/x86/kernel/process_64.c +0 -0

index 607b782afe2c..9e6f142b5cef 100644
@@ -837,8 +837,15 @@ unsigned long KSTK_ESP(struct task_struct *task)
 }
 
 int arch_parse_elf_property(u32 type, const void *data, size_t datasz,

Quoted 31 lines · Yu-cheng

Quoted 10 lines · Dave Martin

Quoted 5 lines · Mark Brown

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

So far so good, but can we also drop the has_interp argument from arch_parse_elf_properties()?

Quoted 8 lines

Cross-check with Yu-Cheng Yu's series, but I don't see this being used any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally to each other now, I don't think we should need a has_interp argument to pass knowledge between the interpreter and executable handling phases here.

For CET, arch_parse_elf_property() needs to know has_interp and is_interp. Like the following, on top of your patches:

arch/x86/kernel/process_64.c +0 -0

index 607b782afe2c..9e6f142b5cef 100644
@@ -837,8 +837,15 @@ unsigned long KSTK_ESP(struct task_struct *task)
  }
 
  int arch_parse_elf_property(u32 type, const void *data, size_t datasz,

arch/x86/kernel/process_64.c unsigned long KSTK_ESP(struct task_struct *task) · line 848

Quoted 10 lines

arch/x86/kernel/process_64.c +8 -1

-			    bool compat, struct arch_elf_state *state)
+			    bool compat, bool has_interp, bool is_interp,
+			    struct arch_elf_state *state)
  {
+	/*
+	 * Parse static-linked executable or the loader.
+	 */
+	if (has_interp != is_interp)
+		return 0;
+

[...]

Ah, sorry, I did attempt to check this with your series, but I didn't attempt to build it. I must have missed this somehow.

But: does x86 actually need to do this?

For arm64, we've discovered that it is better to treat the ELF interpreter and main executable independently when applying the ELF properties.

So, can x86 actually port away from this? arch_parse_elf_properties() and arch_adjust_elf_prot() would still know whether the interpreter is being considered or not, via the is_interp argument to both functions. This allows interpreter and main executable info to be stashed independently in the arch_elf_state.

If x86 really needs to carry on following the existing model then that's fine, but we should try to keep x86 and arm64 aligned if at all possible.

Quoted 5 lines · Mark Brown

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

So far so good, but can we also drop the has_interp argument from arch_parse_elf_properties()?

Cross-check with Yu-Cheng Yu's series, but I don't see this being used any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally to each other now, I don't think we should need a has_interp argument to pass knowledge between the interpreter and executable handling phases here.

Quoted 11 lines · Dave Martin

Quoted 5 lines · Mark Brown

Since we have added an is_interp flag to arch_parse_elf_property() we can drop the has_interp flag from arch_elf_adjust_prot(), the only user was the arm64 code which no longer needs it and any future users will be able to use arch_parse_elf_properties() to determine if an interpreter is in use.

So far so good, but can we also drop the has_interp argument from arch_parse_elf_properties()?

Quoted 7 lines

Cross-check with Yu-Cheng Yu's series, but I don't see this being used any more (except for passthrough in binfmt_elf.c).

Since we are treating the interpreter and main executable orthogonally to each other now, I don't think we should need a has_interp argument to pass knowledge between the interpreter and executable handling phases here.

My thinking was that it might be useful for handling of some future property in the architecture code to know if there is an interpreter, providing the information at parse time would let it set up whatever is needed. We've been doing this with the arm64 BTI handling and while we're moving away from doing that I could imagine that there may be some other case where it makes sense, and it sounds like CET is one.

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

This interacts poorly with MemoryDenyWriteExecute since that 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. This series resolves this by handling the BTI property for both the interpreter and the main executable.

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 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.

v2:
- Add a patch dropping has_interp from arch_adjust_elf_prot()
- Fix bisection issue with static executables on arm64 in the first patch.

Mark Brown (3):
elf: Allow architectures to parse properties on the main executable
arm64: Enable BTI for main executable as well as the interpreter
elf: Remove has_interp property from arch_adjust_elf_prot()

  arch/arm64/include/asm/elf.h | 13 ++++++++++---
  arch/arm64/kernel/process.c  | 20 +++++++-------------
  fs/binfmt_elf.c              | 29 ++++++++++++++++++++---------
  include/linux/elf.h          |  8 +++++---
  4 files changed, 42 insertions(+), 28 deletions(-)

base-commit: c4681547bcce777daf576925a966ffa824edd09d

Of course, there is a good chance I've messed something up or i'm missing something. I should really validate the /lib/ld-linux behavior itself too. I guess this could just as well be a glibc issue (f34 has glibc 2.33-5 which

Hi,

Quoted 8 lines · Mark Brown

Deployments of BTI on arm64 have run into issues interacting with systemd's MemoryDenyWriteExecute feature. Currently for dynamically linked executables the kernel will only handle architecture specific properties like BTI for the interpreter, the expectation is that the interpreter will then handle any properties on the main executable. For BTI this means remapping the executable segments PROT_EXEC | PROT_BTI.

Quoted 6 lines

This interacts poorly with MemoryDenyWriteExecute since that 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. This series resolves this by handling the BTI property for both the interpreter and the main executable.

I've got a Fedora34 system booting in qemu or a model with BTI enabled. On that system I took the systemd-resolved executable, which is one of the services with MDWE enabled, and replaced a number of the bti's with nops. I expect the service to continue to work with the fedora or mainline 5.13 kernel and it does. If instead I boot with MDWE=no for the service, it should fail to start given either of those kernels, and it does.

Thus, I expect that with his patch applied to 5.13 the service will fail to start regardless of the state of MDWE, but it seems to continue starting when I set MDWE=yes. Same behavior with v1 FWTW.

Of course, there is a good chance I've messed something up or i'm missing something. I should really validate the /lib/ld-linux behavior itself too. I guess this could just as well be a glibc issue (f34 has glibc 2.33-5 which appears to have the re-mmap on failure patch). Either way, systemd-resolved is a LSB PIE, with /lib/ld-linux as its interpreter. I've not dug too deep into debugging this, cause I've got a couple other things I need to deal with in the next couple days, and I strongly dislike booting a full debug+system on the model. chuckle, sorry...

Quoted 3 lines · Jeremy Linton via Libc-alpha

Thus, I expect that with his patch applied to 5.13 the service will fail to start regardless of the state of MDWE, but it seems to continue starting when I set MDWE=yes. Same behavior with v1 FWTW.

If the failure we're trying to detect is that BTI is undesirably left off for the main executable, surely replacing BTIs with NOPs will make no differenece? The behaviour with PROT_BTI clear is strictly more permissive than with PROT_BTI set, so I'm not sure we can test the behaviour this way.

Maybe I'm missing sometihng / confused myself somewhere.

Looking at /proc/<pid>/maps after the process starts up may be a more reliable approach, so see what the actual prot value is on the main executable's text pages.

Quoted 11 lines · Dave Martin

Quoted 3 lines · Jeremy Linton via Libc-alpha

Thus, I expect that with his patch applied to 5.13 the service will fail to start regardless of the state of MDWE, but it seems to continue starting when I set MDWE=yes. Same behavior with v1 FWTW.

If the failure we're trying to detect is that BTI is undesirably left off for the main executable, surely replacing BTIs with NOPs will make no differenece? The behaviour with PROT_BTI clear is strictly more permissive than with PROT_BTI set, so I'm not sure we can test the behaviour this way.

Quoted 1 line

Maybe I'm missing sometihng / confused myself somewhere.

The issue this patch series is intended to address is that BTI gets left off since the dynamic linker is unable to enable PROT_BTI on the main executable. We're looking to see that we end up with the stricter permissions checking of BTI, with the issue present landing pads replaced by NOPs will not fault but once the issue is addressed they should start faulting.

Quoted 3 lines

Looking at /proc/<pid>/maps after the process starts up may be a more reliable approach, so see what the actual prot value is on the main executable's text pages.

smaps rather than maps but yes, executable pages show up as "ex" and BTI adds a "bt" tag in VmFlags.

Quoted 13 lines · Mark Brown via Libc-alpha

Quoted 11 lines · Dave Martin

Quoted 3 lines · Jeremy Linton via Libc-alpha

Thus, I expect that with his patch applied to 5.13 the service will fail to start regardless of the state of MDWE, but it seems to continue starting when I set MDWE=yes. Same behavior with v1 FWTW.

If the failure we're trying to detect is that BTI is undesirably left off for the main executable, surely replacing BTIs with NOPs will make no differenece? The behaviour with PROT_BTI clear is strictly more permissive than with PROT_BTI set, so I'm not sure we can test the behaviour this way.

Quoted 8 lines

Quoted 1 line

Maybe I'm missing sometihng / confused myself somewhere.

The issue this patch series is intended to address is that BTI gets left off since the dynamic linker is unable to enable PROT_BTI on the main executable. We're looking to see that we end up with the stricter permissions checking of BTI, with the issue present landing pads replaced by NOPs will not fault but once the issue is addressed they should start faulting.

Ah, right -- I got the test backwards in my head. Yes, that sounds reasonable.

Quoted 7 lines

Quoted 4 lines

Looking at /proc/<pid>/maps after the process starts up may be a more reliable approach, so see what the actual prot value is on the main executable's text pages.

smaps rather than maps but yes, executable pages show up as "ex" and BTI adds a "bt" tag in VmFlags.

Fumbled that -- yes, I meant smaps!

Ignore me...

Cheers
---Dave