[PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

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

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

[PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

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

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


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

[PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Mark Brown]

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 <broonie@kernel.org>
---
 arch/arm64/include/asm/elf.h |  3 ++-
 fs/binfmt_elf.c              | 27 +++++++++++++++++++--------
 include/linux/elf.h          |  4 +++-
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 8d1c8dcb87fd..a488a1329b16 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -261,6 +261,7 @@ struct arch_elf_state {
 
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
+					  bool has_interp, bool is_interp,
 					  struct arch_elf_state *arch)
 {
 	/* No known properties for AArch32 yet */
@@ -273,7 +274,7 @@ static inline int arch_parse_elf_property(u32 type, const void *data,
 		if (datasz != sizeof(*p))
 			return -ENOEXEC;
 
-		if (system_supports_bti() &&
+		if (system_supports_bti() && has_interp == is_interp &&
 		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
 			arch->flags |= ARM64_ELF_BTI;
 	}
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 187b3f2b9202..253ca9969345 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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,
+					      true, true, &arch_state);
+		if (retval)
+			goto out_free_dentry;
+
 	}
 
-	retval = parse_elf_properties(interpreter ?: bprm->file,
-				      elf_property_phdata, &arch_state);
+	retval = parse_elf_properties(bprm->file, elf_property_phdata,
+				      interpreter, false, &arch_state);
 	if (retval)
 		goto out_free_dentry;
 
diff --git a/include/linux/elf.h b/include/linux/elf.h
index c9a46c4e183b..1c45ecf29147 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -88,13 +88,15 @@ struct arch_elf_state;
 #ifndef CONFIG_ARCH_USE_GNU_PROPERTY
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
+					  bool has_interp, bool is_interp,
 					  struct arch_elf_state *arch)
 {
 	return 0;
 }
 #else
 extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
-				   bool compat, struct arch_elf_state *arch);
+				   bool compat, bool has_interp, bool is_interp,
+				   struct arch_elf_state *arch);
 #endif
 
 #ifdef CONFIG_ARCH_HAVE_ELF_PROT
-- 
2.20.1

[PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Mark Brown]

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 <broonie@kernel.org>
---
 arch/arm64/include/asm/elf.h |  3 ++-
 fs/binfmt_elf.c              | 27 +++++++++++++++++++--------
 include/linux/elf.h          |  4 +++-
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 8d1c8dcb87fd..a488a1329b16 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -261,6 +261,7 @@ struct arch_elf_state {
 
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
+					  bool has_interp, bool is_interp,
 					  struct arch_elf_state *arch)
 {
 	/* No known properties for AArch32 yet */
@@ -273,7 +274,7 @@ static inline int arch_parse_elf_property(u32 type, const void *data,
 		if (datasz != sizeof(*p))
 			return -ENOEXEC;
 
-		if (system_supports_bti() &&
+		if (system_supports_bti() && has_interp == is_interp &&
 		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
 			arch->flags |= ARM64_ELF_BTI;
 	}
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 187b3f2b9202..253ca9969345 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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,
+					      true, true, &arch_state);
+		if (retval)
+			goto out_free_dentry;
+
 	}
 
-	retval = parse_elf_properties(interpreter ?: bprm->file,
-				      elf_property_phdata, &arch_state);
+	retval = parse_elf_properties(bprm->file, elf_property_phdata,
+				      interpreter, false, &arch_state);
 	if (retval)
 		goto out_free_dentry;
 
diff --git a/include/linux/elf.h b/include/linux/elf.h
index c9a46c4e183b..1c45ecf29147 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -88,13 +88,15 @@ struct arch_elf_state;
 #ifndef CONFIG_ARCH_USE_GNU_PROPERTY
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
+					  bool has_interp, bool is_interp,
 					  struct arch_elf_state *arch)
 {
 	return 0;
 }
 #else
 extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
-				   bool compat, struct arch_elf_state *arch);
+				   bool compat, bool has_interp, bool is_interp,
+				   struct arch_elf_state *arch);
 #endif
 
 #ifdef CONFIG_ARCH_HAVE_ELF_PROT
-- 
2.20.1


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

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

[Mark Brown]

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 <broonie@kernel.org>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
---
 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 a488a1329b16..9f86dbce2680 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() && 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;
+			}
+		}
 	}
 
 	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)
+			prot |= PROT_BTI;
+	}
 
 	return prot;
 }
-- 
2.20.1

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

[Mark Brown]

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 <broonie@kernel.org>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
---
 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 a488a1329b16..9f86dbce2680 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() && 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;
+			}
+		}
 	}
 
 	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)
+			prot |= PROT_BTI;
+	}
 
 	return prot;
 }
-- 
2.20.1


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

[PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

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

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/process.c | 2 +-
 fs/binfmt_elf.c             | 2 +-
 include/linux/elf.h         | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index f7fff4a4c99f..e51c4aa7e048 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -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)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 253ca9969345..1aba4e50e651 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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,
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 1c45ecf29147..d8392531899d 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -101,11 +101,11 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
 
 #ifdef CONFIG_ARCH_HAVE_ELF_PROT
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
-			 bool has_interp, bool is_interp);
+			 bool is_interp);
 #else
 static inline int arch_elf_adjust_prot(int prot,
 				       const struct arch_elf_state *state,
-				       bool has_interp, bool is_interp)
+				       bool is_interp)
 {
 	return prot;
 }
-- 
2.20.1

[PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

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

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/process.c | 2 +-
 fs/binfmt_elf.c             | 2 +-
 include/linux/elf.h         | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index f7fff4a4c99f..e51c4aa7e048 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -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)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 253ca9969345..1aba4e50e651 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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,
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 1c45ecf29147..d8392531899d 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -101,11 +101,11 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
 
 #ifdef CONFIG_ARCH_HAVE_ELF_PROT
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
-			 bool has_interp, bool is_interp);
+			 bool is_interp);
 #else
 static inline int arch_elf_adjust_prot(int prot,
 				       const struct arch_elf_state *state,
-				       bool has_interp, bool is_interp)
+				       bool is_interp)
 {
 	return prot;
 }
-- 
2.20.1


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

Re: [PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:48PM +0100, Mark Brown wrote:
> 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 <broonie@kernel.org>

I haven't got my head around whether we must move some of the arm64
logic from arch_elf_adjust_proc() to arch_parse_elf_properties(), but
either way, it would make sense to explain the direction of travel here.

(This may just be me failing to keep track of what's changing -- the
affected logic is retained for bisectability here and then dropped later
on in the series...)

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.

[...]

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 187b3f2b9202..253ca9969345 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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,
> +					      true, true, &arch_state);
> +		if (retval)
> +			goto out_free_dentry;
> +
>  	}
>  
> -	retval = parse_elf_properties(interpreter ?: bprm->file,
> -				      elf_property_phdata, &arch_state);
> +	retval = parse_elf_properties(bprm->file, elf_property_phdata,
> +				      interpreter, false, &arch_state);

Nit: interpreter != NULL?

(I guess it works as-is, but from the way this is written it looks a
little like we intend parse_elf_properties() to examine / modify
*interpreter, which is not the case.

>  	if (retval)
>  		goto out_free_dentry;
>  

[...]

Cheers
---Dave

Re: [PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:48PM +0100, Mark Brown wrote:
> 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 <broonie@kernel.org>

I haven't got my head around whether we must move some of the arm64
logic from arch_elf_adjust_proc() to arch_parse_elf_properties(), but
either way, it would make sense to explain the direction of travel here.

(This may just be me failing to keep track of what's changing -- the
affected logic is retained for bisectability here and then dropped later
on in the series...)

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.

[...]

> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 187b3f2b9202..253ca9969345 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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,
> +					      true, true, &arch_state);
> +		if (retval)
> +			goto out_free_dentry;
> +
>  	}
>  
> -	retval = parse_elf_properties(interpreter ?: bprm->file,
> -				      elf_property_phdata, &arch_state);
> +	retval = parse_elf_properties(bprm->file, elf_property_phdata,
> +				      interpreter, false, &arch_state);

Nit: interpreter != NULL?

(I guess it works as-is, but from the way this is written it looks a
little like we intend parse_elf_properties() to examine / modify
*interpreter, which is not the case.

>  	if (retval)
>  		goto out_free_dentry;
>  

[...]

Cheers
---Dave

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

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

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:49PM +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.
> 
> 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 <broonie@kernel.org>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  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 a488a1329b16..9f86dbce2680 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() && 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...)

> +		}
>  	}
>  
>  	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)
> +			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.

Also, due to symmetry between arch_elf_adjust_prot() and
arch_parse_elf_properties() here, could we have something like

	static inline int arm64_elf_bti_flag(bool is_interp)
	{
		if (is_interp)
			return ARM64_ELF_INTERP_BTI;
		else
			return ARM64_ELF_EXEC_BTI;
	}

and then have code like

	if (state->flags & arm64_elf_bti_flag(is_interp))
		prot |= PROT_BTI;

here (with analogous code in arch_elf_adjust_prot()).

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

Either way, these comments are all pretty much cosmetic, and my
Reviewed-by stands.

Cheers
---Dave

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

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:49PM +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.
> 
> 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 <broonie@kernel.org>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  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 a488a1329b16..9f86dbce2680 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() && 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...)

> +		}
>  	}
>  
>  	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)
> +			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.

Also, due to symmetry between arch_elf_adjust_prot() and
arch_parse_elf_properties() here, could we have something like

	static inline int arm64_elf_bti_flag(bool is_interp)
	{
		if (is_interp)
			return ARM64_ELF_INTERP_BTI;
		else
			return ARM64_ELF_EXEC_BTI;
	}

and then have code like

	if (state->flags & arm64_elf_bti_flag(is_interp))
		prot |= PROT_BTI;

here (with analogous code in arch_elf_adjust_prot()).

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

Either way, these comments are all pretty much cosmetic, and my
Reviewed-by stands.

Cheers
---Dave

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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> 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.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 2 +-
>  fs/binfmt_elf.c             | 2 +-
>  include/linux/elf.h         | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index f7fff4a4c99f..e51c4aa7e048 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -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)
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 253ca9969345..1aba4e50e651 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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,
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index 1c45ecf29147..d8392531899d 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -101,11 +101,11 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
>  
>  #ifdef CONFIG_ARCH_HAVE_ELF_PROT
>  int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> -			 bool has_interp, bool is_interp);
> +			 bool is_interp);
>  #else
>  static inline int arch_elf_adjust_prot(int prot,
>  				       const struct arch_elf_state *state,
> -				       bool has_interp, bool is_interp)
> +				       bool is_interp)

[...]

Otherwise, looks reasonable.

Cheers
---Dave

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> 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.

> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/process.c | 2 +-
>  fs/binfmt_elf.c             | 2 +-
>  include/linux/elf.h         | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index f7fff4a4c99f..e51c4aa7e048 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -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)
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 253ca9969345..1aba4e50e651 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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,
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index 1c45ecf29147..d8392531899d 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -101,11 +101,11 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
>  
>  #ifdef CONFIG_ARCH_HAVE_ELF_PROT
>  int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
> -			 bool has_interp, bool is_interp);
> +			 bool is_interp);
>  #else
>  static inline int arch_elf_adjust_prot(int prot,
>  				       const struct arch_elf_state *state,
> -				       bool has_interp, bool is_interp)
> +				       bool is_interp)

[...]

Otherwise, looks reasonable.

Cheers
---Dave

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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Yu, Yu-cheng]

On 6/9/2021 8:17 AM, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>> 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.
> 

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

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 607b782afe2c..9e6f142b5cef 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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,
-			    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;
+
  	if (type != GNU_PROPERTY_X86_FEATURE_1_AND)
  		return 0;

Thanks,
Yu-cheng

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Yu, Yu-cheng]

On 6/9/2021 8:17 AM, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>> 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.
> 

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

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 607b782afe2c..9e6f142b5cef 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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,
-			    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;
+
  	if (type != GNU_PROPERTY_X86_FEATURE_1_AND)
  		return 0;

Thanks,
Yu-cheng

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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
> On 6/9/2021 8:17 AM, Dave Martin wrote:
> >On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> >>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.
> >
> 
> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
> Like the following, on top of your patches:
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 607b782afe2c..9e6f142b5cef 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -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,
> -			    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.

Cheers
---Dave

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
> On 6/9/2021 8:17 AM, Dave Martin wrote:
> >On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> >>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.
> >
> 
> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
> Like the following, on top of your patches:
> 
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 607b782afe2c..9e6f142b5cef 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -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,
> -			    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.

Cheers
---Dave

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

On Wed, Jun 09, 2021 at 04:17:13PM +0100, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:49PM +0100, Mark Brown wrote:

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1436 bytes --]

On Wed, Jun 09, 2021 at 04:17:13PM +0100, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:49PM +0100, Mark Brown wrote:

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

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

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1314 bytes --]

On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:

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

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

On Wed, Jun 09, 2021 at 04:16:25PM +0100, Dave Martin wrote:

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

Yes, it's purely to preserve bisectability.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] elf: Allow architectures to parse properties on the main executable

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 350 bytes --]

On Wed, Jun 09, 2021 at 04:16:25PM +0100, Dave Martin wrote:

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

Yes, it's purely to preserve bisectability.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

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

[Dave Martin]

On Thu, Jun 10, 2021 at 02:19:05PM +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 04:17:13PM +0100, Dave Martin wrote:
> > On Fri, Jun 04, 2021 at 12:24:49PM +0100, Mark Brown wrote:
> 
> > > -		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.

So long as there is a reason rather than it being purely an accident of
editing, that's fine.

(Though if the nested if can be flattened so that this becomes a non-
issue, that's good too :)

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

I was vaguely surprised too -- though I didn't try to compile this
myself yet.  Anyway, not a huge deal.  Adding a helper to generate the
appropriate mask would make this issue go away in any case, but so long
as you're confident this is being evaluated as intended I can take your
word for it.

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

Ack -- again, this was just a suggestion.  I can also live with your
original code if you ultimately decide to stick with that.

Cheers
---Dave

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

[Dave Martin]

On Thu, Jun 10, 2021 at 02:19:05PM +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 04:17:13PM +0100, Dave Martin wrote:
> > On Fri, Jun 04, 2021 at 12:24:49PM +0100, Mark Brown wrote:
> 
> > > -		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.

So long as there is a reason rather than it being purely an accident of
editing, that's fine.

(Though if the nested if can be flattened so that this becomes a non-
issue, that's good too :)

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

I was vaguely surprised too -- though I didn't try to compile this
myself yet.  Anyway, not a huge deal.  Adding a helper to generate the
appropriate mask would make this issue go away in any case, but so long
as you're confident this is being evaluated as intended I can take your
word for it.

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

Ack -- again, this was just a suggestion.  I can also live with your
original code if you ultimately decide to stick with that.

Cheers
---Dave

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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Thu, Jun 10, 2021 at 02:34:03PM +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> > On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> 
> > > 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.
> 
> 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.

It might be useful, but if the use cases are purely hypothetical then it
would still seem preferable to remove it so that it doesn't just hang
around forever as cruft.

I guess we need to wait and see whether x86 really needs this, or just
uses it as a side-effect of following the arm64 code initially.

If it is quicker to stabilise this series heeping the has_interps in,
then I guess we have the option to do that and remove them later on once
we're satisfied they're unlikely to be useful (or not).

Cheers
---Dave

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Dave Martin]

On Thu, Jun 10, 2021 at 02:34:03PM +0100, Mark Brown wrote:
> On Wed, Jun 09, 2021 at 04:17:24PM +0100, Dave Martin wrote:
> > On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
> 
> > > 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.
> 
> 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.

It might be useful, but if the use cases are purely hypothetical then it
would still seem preferable to remove it so that it doesn't just hang
around forever as cruft.

I guess we need to wait and see whether x86 really needs this, or just
uses it as a side-effect of following the arm64 code initially.

If it is quicker to stabilise this series heeping the has_interps in,
then I guess we have the option to do that and remove them later on once
we're satisfied they're unlikely to be useful (or not).

Cheers
---Dave

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Jeremy Linton]

Hi,

On 6/4/21 6:24 AM, Mark Brown wrote:
> 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.

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


Thanks,


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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Jeremy Linton]

Hi,

On 6/4/21 6:24 AM, Mark Brown wrote:
> 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.

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


Thanks,


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


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

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Yu, Yu-cheng]

On 6/10/2021 2:58 AM, Dave Martin wrote:
> On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
>> On 6/9/2021 8:17 AM, Dave Martin wrote:
>>> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>>>> 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.
>>>
>>
>> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
>> Like the following, on top of your patches:
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 607b782afe2c..9e6f142b5cef 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -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,
>> -			    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.
>

Yes, for CET's purpose, that should be fine.

Thanks,
Yu-cheng

Re: [PATCH v2 3/3] elf: Remove has_interp property from arch_adjust_elf_prot()

[Yu, Yu-cheng]

On 6/10/2021 2:58 AM, Dave Martin wrote:
> On Wed, Jun 09, 2021 at 09:55:36AM -0700, Yu, Yu-cheng wrote:
>> On 6/9/2021 8:17 AM, Dave Martin wrote:
>>> On Fri, Jun 04, 2021 at 12:24:50PM +0100, Mark Brown wrote:
>>>> 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.
>>>
>>
>> For CET, arch_parse_elf_property() needs to know has_interp and is_interp.
>> Like the following, on top of your patches:
>>
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 607b782afe2c..9e6f142b5cef 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -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,
>> -			    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.
>

Yes, for CET's purpose, that should be fine.

Thanks,
Yu-cheng

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton wrote:

> 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

If it were a glibc issue that'd mean that glibc would have to somehow
manage to disable PROT_BTI after the kernel set it.  I think I've found
the issue, will send a new version out shortly - we just weren't
actually parsing the properties on the main executable properly.  A new
version should appear shortly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 607 bytes --]

On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton wrote:

> 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

If it were a glibc issue that'd mean that glibc would have to somehow
manage to disable PROT_BTI after the kernel set it.  I think I've found
the issue, will send a new version out shortly - we just weren't
actually parsing the properties on the main executable properly.  A new
version should appear shortly.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Dave Martin]

On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
> Hi,
> 
> On 6/4/21 6:24 AM, Mark Brown wrote:
> >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.
> 
> 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...

[...]

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.

Cheers
---Dave

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Dave Martin]

On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
> Hi,
> 
> On 6/4/21 6:24 AM, Mark Brown wrote:
> >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.
> 
> 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...

[...]

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.

Cheers
---Dave

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
> On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:

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

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1359 bytes --]

On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
> On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:

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

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.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Dave Martin]

On Tue, Jun 15, 2021 at 04:33:41PM +0100, Mark Brown via Libc-alpha wrote:
> On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
> > On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
> 
> > > 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.
> 
> 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.

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Dave Martin]

On Tue, Jun 15, 2021 at 04:33:41PM +0100, Mark Brown via Libc-alpha wrote:
> On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
> > On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
> 
> > > 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.
> 
> 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.

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

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Jeremy Linton]

Hi,

On 6/15/21 10:41 AM, Dave Martin wrote:
> On Tue, Jun 15, 2021 at 04:33:41PM +0100, Mark Brown via Libc-alpha wrote:
>> On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
>>> On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
>>
>>>> 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.
>>
>> 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.

Yes, the good thing about doing both the success and failure cases 
rather than just checking smaps is that one can be assured the emulation 
env and all the pieces are working correctly, not just the mappings,


Anyway, it looks like v3 is behaving as expected, I'm going to let it 
run a few more tests and presumably post a tested-by on the set tomorrow.


Thanks,

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

Re: [PATCH v2 0/3] arm64: Enable BTI for the executable as well as the interpreter

[Jeremy Linton]

Hi,

On 6/15/21 10:41 AM, Dave Martin wrote:
> On Tue, Jun 15, 2021 at 04:33:41PM +0100, Mark Brown via Libc-alpha wrote:
>> On Tue, Jun 15, 2021 at 04:22:06PM +0100, Dave Martin wrote:
>>> On Thu, Jun 10, 2021 at 11:28:12AM -0500, Jeremy Linton via Libc-alpha wrote:
>>
>>>> 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.
>>
>> 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.

Yes, the good thing about doing both the success and failure cases 
rather than just checking smaps is that one can be assured the emulation 
env and all the pieces are working correctly, not just the mappings,


Anyway, it looks like v3 is behaving as expected, I'm going to let it 
run a few more tests and presumably post a tested-by on the set tomorrow.


Thanks,

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


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