All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-19 10:51 ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	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
providing a sysctl which when enabled will cause the kernel to handle
the BTI property for both the interpreter and the main executable.

v13:
 - Rebase onto v5.18-rc3.
v12:
 - Rebase onto v5.18-rc1.
v11:
 - Ignore extra PT_INTERPs.
v10:
 - Add a sysctl abi.bti_main controlling the new behaviour.
v9:
 - Rebase onto v5.17-rc3.
v8:
 - Rebase onto v5.17-rc1.
v7:
 - Rebase onto v5.16-rc1.
v6:
 - Rebase onto v5.15-rc1.
v5:
 - Rebase onto v5.14-rc2.
 - Tweak changelog on patch 1.
 - Use the helper for interpreter/executable flag in elf.h as well.
v4:
 - Rebase onto v5.14-rc1.
v3:
 - Fix passing of properties for parsing by the main executable.
 - Drop has_interp from arch_parse_elf_property().
 - Coding style tweaks.
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 (2):
  elf: Allow architectures to parse properties on the main executable
  arm64: Enable BTI for main executable as well as the interpreter

 arch/arm64/include/asm/elf.h | 14 ++++++++--
 arch/arm64/kernel/process.c  | 52 +++++++++++++++++++++++++++---------
 fs/binfmt_elf.c              | 34 ++++++++++++++++-------
 include/linux/elf.h          |  4 ++-
 4 files changed, 78 insertions(+), 26 deletions(-)


base-commit: b2d229d4ddb17db541098b83524d901257e93845
-- 
2.30.2


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-19 10:51 ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	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
providing a sysctl which when enabled will cause the kernel to handle
the BTI property for both the interpreter and the main executable.

v13:
 - Rebase onto v5.18-rc3.
v12:
 - Rebase onto v5.18-rc1.
v11:
 - Ignore extra PT_INTERPs.
v10:
 - Add a sysctl abi.bti_main controlling the new behaviour.
v9:
 - Rebase onto v5.17-rc3.
v8:
 - Rebase onto v5.17-rc1.
v7:
 - Rebase onto v5.16-rc1.
v6:
 - Rebase onto v5.15-rc1.
v5:
 - Rebase onto v5.14-rc2.
 - Tweak changelog on patch 1.
 - Use the helper for interpreter/executable flag in elf.h as well.
v4:
 - Rebase onto v5.14-rc1.
v3:
 - Fix passing of properties for parsing by the main executable.
 - Drop has_interp from arch_parse_elf_property().
 - Coding style tweaks.
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 (2):
  elf: Allow architectures to parse properties on the main executable
  arm64: Enable BTI for main executable as well as the interpreter

 arch/arm64/include/asm/elf.h | 14 ++++++++--
 arch/arm64/kernel/process.c  | 52 +++++++++++++++++++++++++++---------
 fs/binfmt_elf.c              | 34 ++++++++++++++++-------
 include/linux/elf.h          |  4 ++-
 4 files changed, 78 insertions(+), 26 deletions(-)


base-commit: b2d229d4ddb17db541098b83524d901257e93845
-- 
2.30.2


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v13 1/2] elf: Allow architectures to parse properties on the main executable
  2022-04-19 10:51 ` Mark Brown
@ 2022-04-19 10:51   ` Mark Brown
  -1 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	Mark Brown, Dave Martin

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 a new
flag is_interp flag as with arch_elf_adjust_prot() and calling it for
both the main executable and any intepreter.

The user of this code, arm64, is adapted to ensure that there is no
functional change.

Signed-off-by: Mark Brown <broonie@kernel.org>
Tested-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/elf.h |  3 ++-
 fs/binfmt_elf.c              | 34 ++++++++++++++++++++++++----------
 include/linux/elf.h          |  4 +++-
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 97932fbf973d..5cc002376abe 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -259,6 +259,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 */
@@ -271,7 +272,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 63c7ebb0da89..ed09918d6d69 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -717,8 +717,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;
@@ -752,7 +753,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;
 
@@ -765,6 +767,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 {
@@ -814,7 +817,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);
 
@@ -829,6 +833,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;
@@ -866,12 +871,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) {
 		char *elf_interpreter;
 
+		if (interpreter && elf_property_phdata)
+			break;
+
 		if (elf_ppnt->p_type == PT_GNU_PROPERTY) {
 			elf_property_phdata = elf_ppnt;
 			continue;
 		}
 
-		if (elf_ppnt->p_type != PT_INTERP)
+		if (interpreter || elf_ppnt->p_type != PT_INTERP)
 			continue;
 
 		/*
@@ -920,7 +928,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		if (retval < 0)
 			goto out_free_dentry;
 
-		break;
+		continue;
 
 out_free_interp:
 		kfree(elf_interpreter);
@@ -964,12 +972,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:
@@ -980,10 +987,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.30.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v13 1/2] elf: Allow architectures to parse properties on the main executable
@ 2022-04-19 10:51   ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	Mark Brown, Dave Martin

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 a new
flag is_interp flag as with arch_elf_adjust_prot() and calling it for
both the main executable and any intepreter.

The user of this code, arm64, is adapted to ensure that there is no
functional change.

Signed-off-by: Mark Brown <broonie@kernel.org>
Tested-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/elf.h |  3 ++-
 fs/binfmt_elf.c              | 34 ++++++++++++++++++++++++----------
 include/linux/elf.h          |  4 +++-
 3 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 97932fbf973d..5cc002376abe 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -259,6 +259,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 */
@@ -271,7 +272,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 63c7ebb0da89..ed09918d6d69 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -717,8 +717,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;
@@ -752,7 +753,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;
 
@@ -765,6 +767,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 {
@@ -814,7 +817,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);
 
@@ -829,6 +833,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;
@@ -866,12 +871,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	for (i = 0; i < elf_ex->e_phnum; i++, elf_ppnt++) {
 		char *elf_interpreter;
 
+		if (interpreter && elf_property_phdata)
+			break;
+
 		if (elf_ppnt->p_type == PT_GNU_PROPERTY) {
 			elf_property_phdata = elf_ppnt;
 			continue;
 		}
 
-		if (elf_ppnt->p_type != PT_INTERP)
+		if (interpreter || elf_ppnt->p_type != PT_INTERP)
 			continue;
 
 		/*
@@ -920,7 +928,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		if (retval < 0)
 			goto out_free_dentry;
 
-		break;
+		continue;
 
 out_free_interp:
 		kfree(elf_interpreter);
@@ -964,12 +972,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:
@@ -980,10 +987,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.30.2


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v13 2/2] arm64: Enable BTI for main executable as well as the interpreter
  2022-04-19 10:51 ` Mark Brown
@ 2022-04-19 10:51   ` Mark Brown
  -1 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	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 adding a sysctl abi.bti_main which causes the kernel to
checking the BTI property for the main executable and enable BTI if it
is present when doing the initial mapping. This sysctl is disabled by
default.

Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/elf.h | 15 ++++++++---
 arch/arm64/kernel/process.c  | 52 +++++++++++++++++++++++++++---------
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 5cc002376abe..c4aa60db76a4 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -251,12 +251,21 @@ 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,				\
 }
 
+static inline int arm64_elf_bti_flag(bool is_interp)
+{
+	if (is_interp)
+		return ARM64_ELF_INTERP_BTI;
+	else
+		return ARM64_ELF_EXEC_BTI;
+}
+
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
 					  bool has_interp, bool is_interp,
@@ -272,9 +281,9 @@ 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 &&
+		if (system_supports_bti() &&
 		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
-			arch->flags |= ARM64_ELF_BTI;
+			arch->flags |= arm64_elf_bti_flag(is_interp);
 	}
 
 	return 0;
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7fa97df55e3a..665d375cc3f1 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -702,23 +702,49 @@ core_initcall(tagged_addr_init);
 #endif	/* CONFIG_ARM64_TAGGED_ADDR_ABI */
 
 #ifdef CONFIG_BINFMT_ELF
+static unsigned int bti_main;
+
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
 			 bool has_interp, bool is_interp)
 {
-	/*
-	 * For dynamically linked executables the interpreter is
-	 * responsible for setting PROT_BTI on everything except
-	 * itself.
-	 */
-	if (is_interp != has_interp)
-		return prot;
-
-	if (!(state->flags & ARM64_ELF_BTI))
-		return prot;
-
-	if (prot & PROT_EXEC)
+	if ((prot & PROT_EXEC) &&
+	    (is_interp || !has_interp || bti_main) &&
+	    (state->flags & arm64_elf_bti_flag(is_interp)))
 		prot |= PROT_BTI;
 
 	return prot;
 }
-#endif
+
+#ifdef CONFIG_ARM64_BTI
+/*
+ * If this sysctl is enabled then we will apply PROT_BTI to the main
+ * executable as well as the dynamic linker if it has the appropriate
+ * ELF note.  It is disabled by default, in which case we will only
+ * apply PROT_BTI to the dynamic linker or static binaries.
+ */
+static struct ctl_table bti_main_sysctl_table[] = {
+	{
+		.procname	= "bti_main",
+		.mode		= 0644,
+		.data		= &bti_main,
+		.maxlen		= sizeof(int),
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{ }
+};
+
+static int __init bti_main_init(void)
+{
+	if (!system_supports_bti())
+		return 0;
+
+	if (!register_sysctl("abi", bti_main_sysctl_table))
+		return -EINVAL;
+	return 0;
+}
+core_initcall(bti_main_init);
+#endif /* CONFIG_ARM64_BTI */
+
+#endif /* CONFIG_BINFMT_ELF */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v13 2/2] arm64: Enable BTI for main executable as well as the interpreter
@ 2022-04-19 10:51   ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-19 10:51 UTC (permalink / raw)
  To: Catalin Marinas, Will Deacon
  Cc: Szabolcs Nagy, Jeremy Linton, H . J . Lu, Yu-cheng Yu, Kees Cook,
	Eric Biederman, linux-arch, linux-arm-kernel, libc-alpha,
	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 adding a sysctl abi.bti_main which causes the kernel to
checking the BTI property for the main executable and enable BTI if it
is present when doing the initial mapping. This sysctl is disabled by
default.

Signed-off-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/elf.h | 15 ++++++++---
 arch/arm64/kernel/process.c  | 52 +++++++++++++++++++++++++++---------
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 5cc002376abe..c4aa60db76a4 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -251,12 +251,21 @@ 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,				\
 }
 
+static inline int arm64_elf_bti_flag(bool is_interp)
+{
+	if (is_interp)
+		return ARM64_ELF_INTERP_BTI;
+	else
+		return ARM64_ELF_EXEC_BTI;
+}
+
 static inline int arch_parse_elf_property(u32 type, const void *data,
 					  size_t datasz, bool compat,
 					  bool has_interp, bool is_interp,
@@ -272,9 +281,9 @@ 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 &&
+		if (system_supports_bti() &&
 		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
-			arch->flags |= ARM64_ELF_BTI;
+			arch->flags |= arm64_elf_bti_flag(is_interp);
 	}
 
 	return 0;
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7fa97df55e3a..665d375cc3f1 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -702,23 +702,49 @@ core_initcall(tagged_addr_init);
 #endif	/* CONFIG_ARM64_TAGGED_ADDR_ABI */
 
 #ifdef CONFIG_BINFMT_ELF
+static unsigned int bti_main;
+
 int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
 			 bool has_interp, bool is_interp)
 {
-	/*
-	 * For dynamically linked executables the interpreter is
-	 * responsible for setting PROT_BTI on everything except
-	 * itself.
-	 */
-	if (is_interp != has_interp)
-		return prot;
-
-	if (!(state->flags & ARM64_ELF_BTI))
-		return prot;
-
-	if (prot & PROT_EXEC)
+	if ((prot & PROT_EXEC) &&
+	    (is_interp || !has_interp || bti_main) &&
+	    (state->flags & arm64_elf_bti_flag(is_interp)))
 		prot |= PROT_BTI;
 
 	return prot;
 }
-#endif
+
+#ifdef CONFIG_ARM64_BTI
+/*
+ * If this sysctl is enabled then we will apply PROT_BTI to the main
+ * executable as well as the dynamic linker if it has the appropriate
+ * ELF note.  It is disabled by default, in which case we will only
+ * apply PROT_BTI to the dynamic linker or static binaries.
+ */
+static struct ctl_table bti_main_sysctl_table[] = {
+	{
+		.procname	= "bti_main",
+		.mode		= 0644,
+		.data		= &bti_main,
+		.maxlen		= sizeof(int),
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{ }
+};
+
+static int __init bti_main_init(void)
+{
+	if (!system_supports_bti())
+		return 0;
+
+	if (!register_sysctl("abi", bti_main_sysctl_table))
+		return -EINVAL;
+	return 0;
+}
+core_initcall(bti_main_init);
+#endif /* CONFIG_ARM64_BTI */
+
+#endif /* CONFIG_BINFMT_ELF */
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-19 10:51 ` Mark Brown
@ 2022-04-20  5:33   ` Kees Cook
  -1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20  5:33 UTC (permalink / raw)
  To: will, broonie, catalin.marinas
  Cc: Kees Cook, linux-arm-kernel, jeremy.linton, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> 
> [...]

Applied to for-next/execve, thanks!

[1/2] elf: Allow architectures to parse properties on the main executable
      https://git.kernel.org/kees/c/b2f2553c8e89
[2/2] arm64: Enable BTI for main executable as well as the interpreter
      https://git.kernel.org/kees/c/b65c760600e2

-- 
Kees Cook


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20  5:33   ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20  5:33 UTC (permalink / raw)
  To: will, broonie, catalin.marinas
  Cc: Kees Cook, linux-arm-kernel, jeremy.linton, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> 
> [...]

Applied to for-next/execve, thanks!

[1/2] elf: Allow architectures to parse properties on the main executable
      https://git.kernel.org/kees/c/b2f2553c8e89
[2/2] arm64: Enable BTI for main executable as well as the interpreter
      https://git.kernel.org/kees/c/b65c760600e2

-- 
Kees Cook


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20  5:33   ` Kees Cook
@ 2022-04-20  9:36     ` Will Deacon
  -1 siblings, 0 replies; 30+ messages in thread
From: Will Deacon @ 2022-04-20  9:36 UTC (permalink / raw)
  To: Kees Cook
  Cc: broonie, catalin.marinas, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > 
> > [...]
> 
> Applied to for-next/execve, thanks!
> 
> [1/2] elf: Allow architectures to parse properties on the main executable
>       https://git.kernel.org/kees/c/b2f2553c8e89
> [2/2] arm64: Enable BTI for main executable as well as the interpreter
>       https://git.kernel.org/kees/c/b65c760600e2

Kees, please can you drop this series while Catalin's alternative solution
is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com

Both series expose new behaviours to userspace and we don't need both.

Thanks,

Will

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20  9:36     ` Will Deacon
  0 siblings, 0 replies; 30+ messages in thread
From: Will Deacon @ 2022-04-20  9:36 UTC (permalink / raw)
  To: Kees Cook
  Cc: broonie, catalin.marinas, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > 
> > [...]
> 
> Applied to for-next/execve, thanks!
> 
> [1/2] elf: Allow architectures to parse properties on the main executable
>       https://git.kernel.org/kees/c/b2f2553c8e89
> [2/2] arm64: Enable BTI for main executable as well as the interpreter
>       https://git.kernel.org/kees/c/b65c760600e2

Kees, please can you drop this series while Catalin's alternative solution
is under discussion (his Reviewed-by preceded the other patches)?

https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com

Both series expose new behaviours to userspace and we don't need both.

Thanks,

Will

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20  9:36     ` Will Deacon
@ 2022-04-20  9:57       ` Catalin Marinas
  -1 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2022-04-20  9:57 UTC (permalink / raw)
  To: Will Deacon
  Cc: Kees Cook, broonie, linux-arm-kernel, jeremy.linton, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> > On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > > 
> > > [...]
> > 
> > Applied to for-next/execve, thanks!
> > 
> > [1/2] elf: Allow architectures to parse properties on the main executable
> >       https://git.kernel.org/kees/c/b2f2553c8e89
> > [2/2] arm64: Enable BTI for main executable as well as the interpreter
> >       https://git.kernel.org/kees/c/b65c760600e2
> 
> Kees, please can you drop this series while Catalin's alternative solution
> is under discussion (his Reviewed-by preceded the other patches)?
> 
> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
> Both series expose new behaviours to userspace and we don't need both.

I agree. Even though the patches have my reviewed-by, I think we should
postpone them until we figure out a better W^X solution that does not
affect BTI (and if we can't, we revisit these patches).

Arguably, the two approaches are complementary but the way this series
turned out is for the BTI on main executable to be default off. I have a
worry that the feature won't get used, so we just carry unnecessary code
in the kernel. Jeremy also found this approach less than ideal:

https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20  9:57       ` Catalin Marinas
  0 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2022-04-20  9:57 UTC (permalink / raw)
  To: Will Deacon
  Cc: Kees Cook, broonie, linux-arm-kernel, jeremy.linton, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> > On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > > 
> > > [...]
> > 
> > Applied to for-next/execve, thanks!
> > 
> > [1/2] elf: Allow architectures to parse properties on the main executable
> >       https://git.kernel.org/kees/c/b2f2553c8e89
> > [2/2] arm64: Enable BTI for main executable as well as the interpreter
> >       https://git.kernel.org/kees/c/b65c760600e2
> 
> Kees, please can you drop this series while Catalin's alternative solution
> is under discussion (his Reviewed-by preceded the other patches)?
> 
> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
> Both series expose new behaviours to userspace and we don't need both.

I agree. Even though the patches have my reviewed-by, I think we should
postpone them until we figure out a better W^X solution that does not
affect BTI (and if we can't, we revisit these patches).

Arguably, the two approaches are complementary but the way this series
turned out is for the BTI on main executable to be default off. I have a
worry that the feature won't get used, so we just carry unnecessary code
in the kernel. Jeremy also found this approach less than ideal:

https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com

-- 
Catalin

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20  9:57       ` Catalin Marinas
@ 2022-04-20 11:57         ` Mark Brown
  -1 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-20 11:57 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, Kees Cook, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

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

On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:

> > Kees, please can you drop this series while Catalin's alternative solution
> > is under discussion (his Reviewed-by preceded the other patches)?

> > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com

> > Both series expose new behaviours to userspace and we don't need both.

> I agree. Even though the patches have my reviewed-by, I think we should
> postpone them until we figure out a better W^X solution that does not
> affect BTI (and if we can't, we revisit these patches).

Indeed.  I had been expecting this to follow the pattern of the previous
nine months or so and be mostly ignored for the time being while
Catalin's new series goes forward.  Now that it's applied it might be
worth keeping the first patch still in case someone else needs it but
the second patch can probably wait.

> Arguably, the two approaches are complementary but the way this series
> turned out is for the BTI on main executable to be default off. I have a
> worry that the feature won't get used, so we just carry unnecessary code
> in the kernel. Jeremy also found this approach less than ideal:

> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com

I'm not sure there was a fundamental concern with the approach there but
rather some pushback on the instance on turning it off by default.

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20 11:57         ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-20 11:57 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Will Deacon, Kees Cook, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch


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

On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:

> > Kees, please can you drop this series while Catalin's alternative solution
> > is under discussion (his Reviewed-by preceded the other patches)?

> > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com

> > Both series expose new behaviours to userspace and we don't need both.

> I agree. Even though the patches have my reviewed-by, I think we should
> postpone them until we figure out a better W^X solution that does not
> affect BTI (and if we can't, we revisit these patches).

Indeed.  I had been expecting this to follow the pattern of the previous
nine months or so and be mostly ignored for the time being while
Catalin's new series goes forward.  Now that it's applied it might be
worth keeping the first patch still in case someone else needs it but
the second patch can probably wait.

> Arguably, the two approaches are complementary but the way this series
> turned out is for the BTI on main executable to be default off. I have a
> worry that the feature won't get used, so we just carry unnecessary code
> in the kernel. Jeremy also found this approach less than ideal:

> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com

I'm not sure there was a fundamental concern with the approach there but
rather some pushback on the instance on turning it off by default.

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20 11:57         ` Mark Brown
@ 2022-04-20 13:39           ` Jeremy Linton
  -1 siblings, 0 replies; 30+ messages in thread
From: Jeremy Linton @ 2022-04-20 13:39 UTC (permalink / raw)
  To: Mark Brown, Catalin Marinas
  Cc: Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools, libc-alpha,
	szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

Hi,

On 4/20/22 06:57, Mark Brown wrote:
> On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
>> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> 
>>> Kees, please can you drop this series while Catalin's alternative solution
>>> is under discussion (his Reviewed-by preceded the other patches)?
> 
>>> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
>>> Both series expose new behaviours to userspace and we don't need both.
> 
>> I agree. Even though the patches have my reviewed-by, I think we should
>> postpone them until we figure out a better W^X solution that does not
>> affect BTI (and if we can't, we revisit these patches).
> 
> Indeed.  I had been expecting this to follow the pattern of the previous
> nine months or so and be mostly ignored for the time being while
> Catalin's new series goes forward.  Now that it's applied it might be
> worth keeping the first patch still in case someone else needs it but
> the second patch can probably wait.
> 
>> Arguably, the two approaches are complementary but the way this series
>> turned out is for the BTI on main executable to be default off. I have a
>> worry that the feature won't get used, so we just carry unnecessary code
>> in the kernel. Jeremy also found this approach less than ideal:
> 
>> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> 
> I'm not sure there was a fundamental concern with the approach there but
> rather some pushback on the instance on turning it off by default.

Right, this one seems to have the smallest impact on systemd as it 
exists today. I would have expected the default to be on, because IMHO 
this set corrects what at first glance just looks like a small 
oversight. I find the ABI questions a bit theoretical, given that this 
should only affect environments that don't exist outside of 
labs/development orgs at this point (aka systemd services on HW that 
implements BTI).


The other approach works, and if the systemd folks are on board with it 
also should solve the underlying problem, but it creates a bit of a 
compatibility problem with existing containers/etc that might exist 
today (although running systemd/services in a container is itself a 
discussion).

So, frankly, I don't see why they aren't complementary. This fixes a bug 
we have today, the other set creates a generic mechanism for the future.

Thanks,


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20 13:39           ` Jeremy Linton
  0 siblings, 0 replies; 30+ messages in thread
From: Jeremy Linton @ 2022-04-20 13:39 UTC (permalink / raw)
  To: Mark Brown, Catalin Marinas
  Cc: Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools, libc-alpha,
	szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

Hi,

On 4/20/22 06:57, Mark Brown wrote:
> On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
>> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> 
>>> Kees, please can you drop this series while Catalin's alternative solution
>>> is under discussion (his Reviewed-by preceded the other patches)?
> 
>>> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
>>> Both series expose new behaviours to userspace and we don't need both.
> 
>> I agree. Even though the patches have my reviewed-by, I think we should
>> postpone them until we figure out a better W^X solution that does not
>> affect BTI (and if we can't, we revisit these patches).
> 
> Indeed.  I had been expecting this to follow the pattern of the previous
> nine months or so and be mostly ignored for the time being while
> Catalin's new series goes forward.  Now that it's applied it might be
> worth keeping the first patch still in case someone else needs it but
> the second patch can probably wait.
> 
>> Arguably, the two approaches are complementary but the way this series
>> turned out is for the BTI on main executable to be default off. I have a
>> worry that the feature won't get used, so we just carry unnecessary code
>> in the kernel. Jeremy also found this approach less than ideal:
> 
>> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> 
> I'm not sure there was a fundamental concern with the approach there but
> rather some pushback on the instance on turning it off by default.

Right, this one seems to have the smallest impact on systemd as it 
exists today. I would have expected the default to be on, because IMHO 
this set corrects what at first glance just looks like a small 
oversight. I find the ABI questions a bit theoretical, given that this 
should only affect environments that don't exist outside of 
labs/development orgs at this point (aka systemd services on HW that 
implements BTI).


The other approach works, and if the systemd folks are on board with it 
also should solve the underlying problem, but it creates a bit of a 
compatibility problem with existing containers/etc that might exist 
today (although running systemd/services in a container is itself a 
discussion).

So, frankly, I don't see why they aren't complementary. This fixes a bug 
we have today, the other set creates a generic mechanism for the future.

Thanks,


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20  9:36     ` Will Deacon
@ 2022-04-20 16:48       ` Kees Cook
  -1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:48 UTC (permalink / raw)
  To: Will Deacon
  Cc: broonie, catalin.marinas, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> > On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > > 
> > > [...]
> > 
> > Applied to for-next/execve, thanks!
> > 
> > [1/2] elf: Allow architectures to parse properties on the main executable
> >       https://git.kernel.org/kees/c/b2f2553c8e89
> > [2/2] arm64: Enable BTI for main executable as well as the interpreter
> >       https://git.kernel.org/kees/c/b65c760600e2
> 
> Kees, please can you drop this series while Catalin's alternative solution
> is under discussion (his Reviewed-by preceded the other patches)?
> 
> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
> Both series expose new behaviours to userspace and we don't need both.

Ah-ha! I wasn't sure if they were solving the same problem or not.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20 16:48       ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:48 UTC (permalink / raw)
  To: Will Deacon
  Cc: broonie, catalin.marinas, linux-arm-kernel, jeremy.linton,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> > On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > > 
> > > [...]
> > 
> > Applied to for-next/execve, thanks!
> > 
> > [1/2] elf: Allow architectures to parse properties on the main executable
> >       https://git.kernel.org/kees/c/b2f2553c8e89
> > [2/2] arm64: Enable BTI for main executable as well as the interpreter
> >       https://git.kernel.org/kees/c/b65c760600e2
> 
> Kees, please can you drop this series while Catalin's alternative solution
> is under discussion (his Reviewed-by preceded the other patches)?
> 
> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> 
> Both series expose new behaviours to userspace and we don't need both.

Ah-ha! I wasn't sure if they were solving the same problem or not.

-- 
Kees Cook

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20 13:39           ` Jeremy Linton
@ 2022-04-20 16:51             ` Kees Cook
  -1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Mark Brown, Catalin Marinas, Will Deacon, linux-arm-kernel,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
> On 4/20/22 06:57, Mark Brown wrote:
> > On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> > > On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> > 
> > > > Kees, please can you drop this series while Catalin's alternative solution
> > > > is under discussion (his Reviewed-by preceded the other patches)?
> > 
> > > > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> > 
> > > > Both series expose new behaviours to userspace and we don't need both.
> > 
> > > I agree. Even though the patches have my reviewed-by, I think we should
> > > postpone them until we figure out a better W^X solution that does not
> > > affect BTI (and if we can't, we revisit these patches).
> > 
> > Indeed.  I had been expecting this to follow the pattern of the previous
> > nine months or so and be mostly ignored for the time being while
> > Catalin's new series goes forward.  Now that it's applied it might be
> > worth keeping the first patch still in case someone else needs it but
> > the second patch can probably wait.
> > 
> > > Arguably, the two approaches are complementary but the way this series
> > > turned out is for the BTI on main executable to be default off. I have a
> > > worry that the feature won't get used, so we just carry unnecessary code
> > > in the kernel. Jeremy also found this approach less than ideal:
> > 
> > > https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> > 
> > I'm not sure there was a fundamental concern with the approach there but
> > rather some pushback on the instance on turning it off by default.
> 
> Right, this one seems to have the smallest impact on systemd as it exists
> today. I would have expected the default to be on, because IMHO this set
> corrects what at first glance just looks like a small oversight. I find the
> ABI questions a bit theoretical, given that this should only affect
> environments that don't exist outside of labs/development orgs at this point
> (aka systemd services on HW that implements BTI).
> 
> 
> The other approach works, and if the systemd folks are on board with it also
> should solve the underlying problem, but it creates a bit of a compatibility
> problem with existing containers/etc that might exist today (although
> running systemd/services in a container is itself a discussion).
> 
> So, frankly, I don't see why they aren't complementary. This fixes a bug we
> have today, the other set creates a generic mechanism for the future.

Okay, well, how about I drop this for now, and I'll Ack the ELF loader
changes so this can go through the arm64 tree if there is consensus.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20 16:51             ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Mark Brown, Catalin Marinas, Will Deacon, linux-arm-kernel,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
> On 4/20/22 06:57, Mark Brown wrote:
> > On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> > > On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> > 
> > > > Kees, please can you drop this series while Catalin's alternative solution
> > > > is under discussion (his Reviewed-by preceded the other patches)?
> > 
> > > > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> > 
> > > > Both series expose new behaviours to userspace and we don't need both.
> > 
> > > I agree. Even though the patches have my reviewed-by, I think we should
> > > postpone them until we figure out a better W^X solution that does not
> > > affect BTI (and if we can't, we revisit these patches).
> > 
> > Indeed.  I had been expecting this to follow the pattern of the previous
> > nine months or so and be mostly ignored for the time being while
> > Catalin's new series goes forward.  Now that it's applied it might be
> > worth keeping the first patch still in case someone else needs it but
> > the second patch can probably wait.
> > 
> > > Arguably, the two approaches are complementary but the way this series
> > > turned out is for the BTI on main executable to be default off. I have a
> > > worry that the feature won't get used, so we just carry unnecessary code
> > > in the kernel. Jeremy also found this approach less than ideal:
> > 
> > > https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> > 
> > I'm not sure there was a fundamental concern with the approach there but
> > rather some pushback on the instance on turning it off by default.
> 
> Right, this one seems to have the smallest impact on systemd as it exists
> today. I would have expected the default to be on, because IMHO this set
> corrects what at first glance just looks like a small oversight. I find the
> ABI questions a bit theoretical, given that this should only affect
> environments that don't exist outside of labs/development orgs at this point
> (aka systemd services on HW that implements BTI).
> 
> 
> The other approach works, and if the systemd folks are on board with it also
> should solve the underlying problem, but it creates a bit of a compatibility
> problem with existing containers/etc that might exist today (although
> running systemd/services in a container is itself a discussion).
> 
> So, frankly, I don't see why they aren't complementary. This fixes a bug we
> have today, the other set creates a generic mechanism for the future.

Okay, well, how about I drop this for now, and I'll Ack the ELF loader
changes so this can go through the arm64 tree if there is consensus.

-- 
Kees Cook

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 1/2] elf: Allow architectures to parse properties on the main executable
  2022-04-19 10:51   ` Mark Brown
@ 2022-04-20 16:51     ` Kees Cook
  -1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Szabolcs Nagy, Jeremy Linton,
	H . J . Lu, Yu-cheng Yu, Eric Biederman, linux-arch,
	linux-arm-kernel, libc-alpha, Dave Martin

On Tue, Apr 19, 2022 at 11:51:55AM +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 a new
> flag is_interp flag as with arch_elf_adjust_prot() and calling it for
> both the main executable and any intepreter.
> 
> The user of this code, arm64, is adapted to ensure that there is no
> functional change.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 1/2] elf: Allow architectures to parse properties on the main executable
@ 2022-04-20 16:51     ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: Catalin Marinas, Will Deacon, Szabolcs Nagy, Jeremy Linton,
	H . J . Lu, Yu-cheng Yu, Eric Biederman, linux-arch,
	linux-arm-kernel, libc-alpha, Dave Martin

On Tue, Apr 19, 2022 at 11:51:55AM +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 a new
> flag is_interp flag as with arch_elf_adjust_prot() and calling it for
> both the main executable and any intepreter.
> 
> The user of this code, arm64, is adapted to ensure that there is no
> functional change.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20  5:33   ` Kees Cook
@ 2022-04-20 16:51     ` Kees Cook
  -1 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: will, broonie, catalin.marinas
  Cc: linux-arm-kernel, jeremy.linton, hjl.tools, libc-alpha,
	szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > 
> > [...]
> 
> Applied to for-next/execve, thanks!

Now un-applied! :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-20 16:51     ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2022-04-20 16:51 UTC (permalink / raw)
  To: will, broonie, catalin.marinas
  Cc: linux-arm-kernel, jeremy.linton, hjl.tools, libc-alpha,
	szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Tue, Apr 19, 2022 at 10:33:06PM -0700, Kees Cook wrote:
> On Tue, 19 Apr 2022 11:51:54 +0100, 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.
> > 
> > [...]
> 
> Applied to for-next/execve, thanks!

Now un-applied! :)

-- 
Kees Cook

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-20 13:39           ` Jeremy Linton
@ 2022-04-21  9:34             ` Catalin Marinas
  -1 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2022-04-21  9:34 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Mark Brown, Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
> On 4/20/22 06:57, Mark Brown wrote:
> > On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> > > On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> > > > Kees, please can you drop this series while Catalin's alternative solution
> > > > is under discussion (his Reviewed-by preceded the other patches)?
> > 
> > > > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> > 
> > > > Both series expose new behaviours to userspace and we don't need both.
[...]
> > > Arguably, the two approaches are complementary but the way this series
> > > turned out is for the BTI on main executable to be default off. I have a
> > > worry that the feature won't get used, so we just carry unnecessary code
> > > in the kernel. Jeremy also found this approach less than ideal:
> > 
> > > https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> > 
> > I'm not sure there was a fundamental concern with the approach there but
> > rather some pushback on the instance on turning it off by default.
> 
> Right, this one seems to have the smallest impact on systemd as it exists
> today.

It had a bigger impact on glibc which had to rework the dynamic library
mapping to use munmap/mmap() instead of an mprotect() (though that's
already done). I think glibc still prefers the mprotect() approach for
dynamic libraries.

> I would have expected the default to be on, because IMHO this set
> corrects what at first glance just looks like a small oversight.

This was a design decision at the time, maybe not the best but it gives
us some flexibility (and we haven't thought of MDWE).

> I find the ABI questions a bit theoretical, given that this should
> only affect environments that don't exist outside of labs/development
> orgs at this point (aka systemd services on HW that implements BTI).

The worry is not what breaks now but rather what happens when today's
distros will eventually be deployed on large-scale BTI-capable hardware.
It's a very small risk but non-zero. The idea is that if we come across
some weird problem, a fixed-up dynamic loader could avoid enabling BTI
on a per-process basis without the need to do this at the system level.

Personally I'm fine with this risk. Will is not and I respect his
position, hence I started the other thread to come up with a MDWE
alternative.

> The other approach works, and if the systemd folks are on board with it also
> should solve the underlying problem, but it creates a bit of a compatibility
> problem with existing containers/etc that might exist today (although
> running systemd/services in a container is itself a discussion).
> 
> So, frankly, I don't see why they aren't complementary.

They are complementary, though if we change the MDWE approach, there's
less of a need for this patchset.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-21  9:34             ` Catalin Marinas
  0 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2022-04-21  9:34 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Mark Brown, Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
> On 4/20/22 06:57, Mark Brown wrote:
> > On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
> > > On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
> > > > Kees, please can you drop this series while Catalin's alternative solution
> > > > is under discussion (his Reviewed-by preceded the other patches)?
> > 
> > > > https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
> > 
> > > > Both series expose new behaviours to userspace and we don't need both.
[...]
> > > Arguably, the two approaches are complementary but the way this series
> > > turned out is for the BTI on main executable to be default off. I have a
> > > worry that the feature won't get used, so we just carry unnecessary code
> > > in the kernel. Jeremy also found this approach less than ideal:
> > 
> > > https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
> > 
> > I'm not sure there was a fundamental concern with the approach there but
> > rather some pushback on the instance on turning it off by default.
> 
> Right, this one seems to have the smallest impact on systemd as it exists
> today.

It had a bigger impact on glibc which had to rework the dynamic library
mapping to use munmap/mmap() instead of an mprotect() (though that's
already done). I think glibc still prefers the mprotect() approach for
dynamic libraries.

> I would have expected the default to be on, because IMHO this set
> corrects what at first glance just looks like a small oversight.

This was a design decision at the time, maybe not the best but it gives
us some flexibility (and we haven't thought of MDWE).

> I find the ABI questions a bit theoretical, given that this should
> only affect environments that don't exist outside of labs/development
> orgs at this point (aka systemd services on HW that implements BTI).

The worry is not what breaks now but rather what happens when today's
distros will eventually be deployed on large-scale BTI-capable hardware.
It's a very small risk but non-zero. The idea is that if we come across
some weird problem, a fixed-up dynamic loader could avoid enabling BTI
on a per-process basis without the need to do this at the system level.

Personally I'm fine with this risk. Will is not and I respect his
position, hence I started the other thread to come up with a MDWE
alternative.

> The other approach works, and if the systemd folks are on board with it also
> should solve the underlying problem, but it creates a bit of a compatibility
> problem with existing containers/etc that might exist today (although
> running systemd/services in a container is itself a discussion).
> 
> So, frankly, I don't see why they aren't complementary.

They are complementary, though if we change the MDWE approach, there's
less of a need for this patchset.

-- 
Catalin

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-21  9:34             ` Catalin Marinas
@ 2022-04-21 15:52               ` Jeremy Linton
  -1 siblings, 0 replies; 30+ messages in thread
From: Jeremy Linton @ 2022-04-21 15:52 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Mark Brown, Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

Hi,

On 4/21/22 04:34, Catalin Marinas wrote:
> On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
>> On 4/20/22 06:57, Mark Brown wrote:
>>> On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
>>>> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
>>>>> Kees, please can you drop this series while Catalin's alternative solution
>>>>> is under discussion (his Reviewed-by preceded the other patches)?
>>>
>>>>> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
>>>
>>>>> Both series expose new behaviours to userspace and we don't need both.
> [...]
>>>> Arguably, the two approaches are complementary but the way this series
>>>> turned out is for the BTI on main executable to be default off. I have a
>>>> worry that the feature won't get used, so we just carry unnecessary code
>>>> in the kernel. Jeremy also found this approach less than ideal:
>>>
>>>> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
>>>
>>> I'm not sure there was a fundamental concern with the approach there but
>>> rather some pushback on the instance on turning it off by default.
>>
>> Right, this one seems to have the smallest impact on systemd as it exists
>> today.
> 
> It had a bigger impact on glibc which had to rework the dynamic library
> mapping to use munmap/mmap() instead of an mprotect() (though that's
> already done). I think glibc still prefers the mprotect() approach for
> dynamic libraries.
> 
>> I would have expected the default to be on, because IMHO this set
>> corrects what at first glance just looks like a small oversight.
> 
> This was a design decision at the time, maybe not the best but it gives
> us some flexibility (and we haven't thought of MDWE).
> 
>> I find the ABI questions a bit theoretical, given that this should
>> only affect environments that don't exist outside of labs/development
>> orgs at this point (aka systemd services on HW that implements BTI).
> 
> The worry is not what breaks now but rather what happens when today's
> distros will eventually be deployed on large-scale BTI-capable hardware.
> It's a very small risk but non-zero. The idea is that if we come across
> some weird problem, a fixed-up dynamic loader could avoid enabling BTI
> on a per-process basis without the need to do this at the system level.
> 
> Personally I'm fine with this risk. Will is not and I respect his
> position, hence I started the other thread to come up with a MDWE
> alternative.

To clarify though, there is already a way to restore process 
functionality to the small subset of services with the MDWE enabled, 
which is simply to flip off MDWE on the service and let some future 
loader flag clear PROT_BTI in the code path it would normally be setting 
PROT_EXE|PROT_BIT on.

Or maybe simpler yet, we provide a tool which wipes out the gnu BTI note 
on binaries that are found to have BTI bugs, thereby (correctly) fixing 
the problem at its source. This is at least presumably doable if we are 
also assuming we can update glibc/etc in any environment with the problem.

But again, systemd MDWE + BTI are less than a dozen processes, and if we 
are worried about the big hammer of disabling BTI system wide, we 
probably should have the ability to disable it per-process rather than 
worrying about this obscure edge case.





> 
>> The other approach works, and if the systemd folks are on board with it also
>> should solve the underlying problem, but it creates a bit of a compatibility
>> problem with existing containers/etc that might exist today (although
>> running systemd/services in a container is itself a discussion).
>>
>> So, frankly, I don't see why they aren't complementary.
> 
> They are complementary, though if we change the MDWE approach, there's
> less of a need for this patchset.
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-21 15:52               ` Jeremy Linton
  0 siblings, 0 replies; 30+ messages in thread
From: Jeremy Linton @ 2022-04-21 15:52 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Mark Brown, Will Deacon, Kees Cook, linux-arm-kernel, hjl.tools,
	libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm, linux-arch

Hi,

On 4/21/22 04:34, Catalin Marinas wrote:
> On Wed, Apr 20, 2022 at 08:39:14AM -0500, Jeremy Linton wrote:
>> On 4/20/22 06:57, Mark Brown wrote:
>>> On Wed, Apr 20, 2022 at 10:57:30AM +0100, Catalin Marinas wrote:
>>>> On Wed, Apr 20, 2022 at 10:36:13AM +0100, Will Deacon wrote:
>>>>> Kees, please can you drop this series while Catalin's alternative solution
>>>>> is under discussion (his Reviewed-by preceded the other patches)?
>>>
>>>>> https://lore.kernel.org/r/20220413134946.2732468-1-catalin.marinas@arm.com
>>>
>>>>> Both series expose new behaviours to userspace and we don't need both.
> [...]
>>>> Arguably, the two approaches are complementary but the way this series
>>>> turned out is for the BTI on main executable to be default off. I have a
>>>> worry that the feature won't get used, so we just carry unnecessary code
>>>> in the kernel. Jeremy also found this approach less than ideal:
>>>
>>>> https://lore.kernel.org/r/59fc8a58-5013-606b-f544-8277cda18e50@arm.com
>>>
>>> I'm not sure there was a fundamental concern with the approach there but
>>> rather some pushback on the instance on turning it off by default.
>>
>> Right, this one seems to have the smallest impact on systemd as it exists
>> today.
> 
> It had a bigger impact on glibc which had to rework the dynamic library
> mapping to use munmap/mmap() instead of an mprotect() (though that's
> already done). I think glibc still prefers the mprotect() approach for
> dynamic libraries.
> 
>> I would have expected the default to be on, because IMHO this set
>> corrects what at first glance just looks like a small oversight.
> 
> This was a design decision at the time, maybe not the best but it gives
> us some flexibility (and we haven't thought of MDWE).
> 
>> I find the ABI questions a bit theoretical, given that this should
>> only affect environments that don't exist outside of labs/development
>> orgs at this point (aka systemd services on HW that implements BTI).
> 
> The worry is not what breaks now but rather what happens when today's
> distros will eventually be deployed on large-scale BTI-capable hardware.
> It's a very small risk but non-zero. The idea is that if we come across
> some weird problem, a fixed-up dynamic loader could avoid enabling BTI
> on a per-process basis without the need to do this at the system level.
> 
> Personally I'm fine with this risk. Will is not and I respect his
> position, hence I started the other thread to come up with a MDWE
> alternative.

To clarify though, there is already a way to restore process 
functionality to the small subset of services with the MDWE enabled, 
which is simply to flip off MDWE on the service and let some future 
loader flag clear PROT_BTI in the code path it would normally be setting 
PROT_EXE|PROT_BIT on.

Or maybe simpler yet, we provide a tool which wipes out the gnu BTI note 
on binaries that are found to have BTI bugs, thereby (correctly) fixing 
the problem at its source. This is at least presumably doable if we are 
also assuming we can update glibc/etc in any environment with the problem.

But again, systemd MDWE + BTI are less than a dozen processes, and if we 
are worried about the big hammer of disabling BTI system wide, we 
probably should have the ability to disable it per-process rather than 
worrying about this obscure edge case.





> 
>> The other approach works, and if the systemd folks are on board with it also
>> should solve the underlying problem, but it creates a bit of a compatibility
>> problem with existing containers/etc that might exist today (although
>> running systemd/services in a container is itself a discussion).
>>
>> So, frankly, I don't see why they aren't complementary.
> 
> They are complementary, though if we change the MDWE approach, there's
> less of a need for this patchset.
> 


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
  2022-04-21 15:52               ` Jeremy Linton
@ 2022-04-21 17:58                 ` Mark Brown
  -1 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-21 17:58 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Catalin Marinas, Will Deacon, Kees Cook, linux-arm-kernel,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch

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

On Thu, Apr 21, 2022 at 10:52:52AM -0500, Jeremy Linton wrote:

> Or maybe simpler yet, we provide a tool which wipes out the gnu BTI note on
> binaries that are found to have BTI bugs, thereby (correctly) fixing the
> problem at its source. This is at least presumably doable if we are also
> assuming we can update glibc/etc in any environment with the problem.

This seems like the most sensible thing if we do find we're running into
BTI executables that are incorrectly annotated and difficult to fix - it
avoids having to manage any new permissions for bypassing BTI.

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter
@ 2022-04-21 17:58                 ` Mark Brown
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Brown @ 2022-04-21 17:58 UTC (permalink / raw)
  To: Jeremy Linton
  Cc: Catalin Marinas, Will Deacon, Kees Cook, linux-arm-kernel,
	hjl.tools, libc-alpha, szabolcs.nagy, yu-cheng.yu, ebiederm,
	linux-arch


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

On Thu, Apr 21, 2022 at 10:52:52AM -0500, Jeremy Linton wrote:

> Or maybe simpler yet, we provide a tool which wipes out the gnu BTI note on
> binaries that are found to have BTI bugs, thereby (correctly) fixing the
> problem at its source. This is at least presumably doable if we are also
> assuming we can update glibc/etc in any environment with the problem.

This seems like the most sensible thing if we do find we're running into
BTI executables that are incorrectly annotated and difficult to fix - it
avoids having to manage any new permissions for bypassing BTI.

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2022-04-21 18:01 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 10:51 [PATCH v13 0/2] arm64: Enable BTI for the executable as well as the interpreter Mark Brown
2022-04-19 10:51 ` Mark Brown
2022-04-19 10:51 ` [PATCH v13 1/2] elf: Allow architectures to parse properties on the main executable Mark Brown
2022-04-19 10:51   ` Mark Brown
2022-04-20 16:51   ` Kees Cook
2022-04-20 16:51     ` Kees Cook
2022-04-19 10:51 ` [PATCH v13 2/2] arm64: Enable BTI for main executable as well as the interpreter Mark Brown
2022-04-19 10:51   ` Mark Brown
2022-04-20  5:33 ` [PATCH v13 0/2] arm64: Enable BTI for the " Kees Cook
2022-04-20  5:33   ` Kees Cook
2022-04-20  9:36   ` Will Deacon
2022-04-20  9:36     ` Will Deacon
2022-04-20  9:57     ` Catalin Marinas
2022-04-20  9:57       ` Catalin Marinas
2022-04-20 11:57       ` Mark Brown
2022-04-20 11:57         ` Mark Brown
2022-04-20 13:39         ` Jeremy Linton
2022-04-20 13:39           ` Jeremy Linton
2022-04-20 16:51           ` Kees Cook
2022-04-20 16:51             ` Kees Cook
2022-04-21  9:34           ` Catalin Marinas
2022-04-21  9:34             ` Catalin Marinas
2022-04-21 15:52             ` Jeremy Linton
2022-04-21 15:52               ` Jeremy Linton
2022-04-21 17:58               ` Mark Brown
2022-04-21 17:58                 ` Mark Brown
2022-04-20 16:48     ` Kees Cook
2022-04-20 16:48       ` Kees Cook
2022-04-20 16:51   ` Kees Cook
2022-04-20 16:51     ` Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.