linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs
@ 2020-02-10 19:30 Kees Cook
  2020-02-10 19:30 ` [PATCH v3 1/7] x86/elf: Add table to document READ_IMPLIES_EXEC Kees Cook
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

Hi,

This is a refresh of my earlier attempt to fix READ_IMPLIES_EXEC. I think
it incorporates the feedback from v2, and I've now added a selftest. This
series is for x86, arm, and arm64; I'd like it to go via -tip, though,
just to keep this change together with the selftest. To that end, I'd like
to collect Acks from the respective architecture maintainers. (Note that
most other architectures don't suffer from this problem. e.g. powerpc's
behavior appears to already be correct. MIPS may need adjusting but the
history of CPU features and toolchain behavior is very unclear to me.)

Repeating the commit log from later in the series:


The READ_IMPLIES_EXEC work-around was designed for old toolchains that
lacked the ELF PT_GNU_STACK marking under the assumption that toolchains
that couldn't specify executable permission flags for the stack may not
know how to do it correctly for any memory region.

This logic is sensible for having ancient binaries coexist in a system
with possibly NX memory, but was implemented in a way that equated having
a PT_GNU_STACK marked executable as being as "broken" as lacking the
PT_GNU_STACK marking entirely. Things like unmarked assembly and stack
trampolines may cause PT_GNU_STACK to need an executable bit, but they
do not imply all mappings must be executable.

This confusion has led to situations where modern programs with explicitly
marked executable stack are forced into the READ_IMPLIES_EXEC state when
no such thing is needed. (And leads to unexpected failures when mmap()ing
regions of device driver memory that wish to disallow VM_EXEC[1].)

In looking for other reasons for the READ_IMPLIES_EXEC behavior, Jann
Horn noted that glibc thread stacks have always been marked RWX (until
2003 when they started tracking the PT_GNU_STACK flag instead[2]). And
musl doesn't support executable stacks at all[3]. As such, no breakage
for multithreaded applications is expected from this change.

[1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
[2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=54ee14b3882
[3] https://lkml.kernel.org/r/20190423192534.GN23599@brightrain.aerifal.cx


-Kees


v3:
 - split steps in to distinct patches
 - include arm32 and arm64/compat
 - add selftests to validate behavior
v2: https://lore.kernel.org/lkml/20190424203408.GA11386@beast/
v1: https://lore.kernel.org/lkml/20190423181210.GA2443@beast/

Kees Cook (7):
  x86/elf: Add table to document READ_IMPLIES_EXEC
  x86/elf: Split READ_IMPLIES_EXEC from executable GNU_STACK
  x86/elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces
  arm32/64, elf: Add tables to document READ_IMPLIES_EXEC
  arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK
  arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address
    spaces
  selftests/exec: Add READ_IMPLIES_EXEC tests

 arch/arm/kernel/elf.c                         |  27 +++-
 arch/arm64/include/asm/elf.h                  |  23 +++-
 arch/x86/include/asm/elf.h                    |  22 +++-
 fs/compat_binfmt_elf.c                        |   5 +
 tools/testing/selftests/exec/Makefile         |  42 +++++-
 .../selftests/exec/read_implies_exec.c        | 121 ++++++++++++++++++
 .../selftests/exec/strip-gnu-stack-bits.c     |  34 +++++
 .../testing/selftests/exec/strip-gnu-stack.c  |  69 ++++++++++
 8 files changed, 336 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/exec/read_implies_exec.c
 create mode 100644 tools/testing/selftests/exec/strip-gnu-stack-bits.c
 create mode 100644 tools/testing/selftests/exec/strip-gnu-stack.c

-- 
2.20.1


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

* [PATCH v3 1/7] x86/elf: Add table to document READ_IMPLIES_EXEC
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-10 19:30 ` [PATCH v3 2/7] x86/elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

Add a table to document the current behavior of READ_IMPLIES_EXEC in
preparation for changing the behavior.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/elf.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 69c0f892e310..733f69c2b053 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -281,6 +281,25 @@ extern u32 elf_hwcap2;
 /*
  * An executable for which elf_read_implies_exec() returns TRUE will
  * have the READ_IMPLIES_EXEC personality flag set automatically.
+ *
+ * The decision process for determining the results are:
+ *
+ *              CPU: | lacks NX*  | has NX, ia32     | has NX, x86_64 |
+ * ELF:              |            |                  |                |
+ * -------------------------------|------------------|----------------|
+ * missing GNU_STACK | exec-all   | exec-all         | exec-all       |
+ * GNU_STACK == RWX  | exec-all   | exec-all         | exec-all       |
+ * GNU_STACK == RW   | exec-none  | exec-none        | exec-none      |
+ *
+ *  exec-all  : all PROT_READ user mappings are executable, except when
+ *              backed by files on a noexec-filesystem.
+ *  exec-none : only PROT_EXEC user mappings are executable.
+ *
+ *  *this column has no architectural effect: NX markings are ignored by
+ *   hardware, but may have behavioral effects when "wants X" collides with
+ *   "cannot be X" constraints in memory permission flags, as in
+ *   https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
+ *
  */
 #define elf_read_implies_exec(ex, executable_stack)	\
 	(executable_stack != EXSTACK_DISABLE_X)
-- 
2.20.1


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

* [PATCH v3 2/7] x86/elf: Split READ_IMPLIES_EXEC from executable GNU_STACK
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
  2020-02-10 19:30 ` [PATCH v3 1/7] x86/elf: Add table to document READ_IMPLIES_EXEC Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-10 19:30 ` [PATCH v3 3/7] x86/elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

The READ_IMPLIES_EXEC work-around was designed for old toolchains that
lacked the ELF PT_GNU_STACK marking under the assumption that toolchains
that couldn't specify executable permission flags for the stack may not
know how to do it correctly for any memory region.

This logic is sensible for having ancient binaries coexist in a system
with possibly NX memory, but was implemented in a way that equated having
a PT_GNU_STACK marked executable as being as "broken" as lacking the
PT_GNU_STACK marking entirely. Things like unmarked assembly and stack
trampolines may cause PT_GNU_STACK to need an executable bit, but they
do not imply all mappings must be executable.

This confusion has led to situations where modern programs with explicitly
marked executable stack are forced into the READ_IMPLIES_EXEC state when
no such thing is needed. (And leads to unexpected failures when mmap()ing
regions of device driver memory that wish to disallow VM_EXEC[1].)

In looking for other reasons for the READ_IMPLIES_EXEC behavior, Jann
Horn noted that glibc thread stacks have always been marked RWX (until
2003 when they started tracking the PT_GNU_STACK flag instead[2]). And
musl doesn't support executable stacks at all[3]. As such, no breakage
for multithreaded applications is expected from this change.

[1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
[2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=54ee14b3882
[3] https://lkml.kernel.org/r/20190423192534.GN23599@brightrain.aerifal.cx

Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/elf.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 733f69c2b053..a7035065377c 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -288,12 +288,13 @@ extern u32 elf_hwcap2;
  * ELF:              |            |                  |                |
  * -------------------------------|------------------|----------------|
  * missing GNU_STACK | exec-all   | exec-all         | exec-all       |
- * GNU_STACK == RWX  | exec-all   | exec-all         | exec-all       |
+ * GNU_STACK == RWX  | exec-stack | exec-stack       | exec-stack     |
  * GNU_STACK == RW   | exec-none  | exec-none        | exec-none      |
  *
  *  exec-all  : all PROT_READ user mappings are executable, except when
  *              backed by files on a noexec-filesystem.
  *  exec-none : only PROT_EXEC user mappings are executable.
+ *  exec-stack: only the stack and PROT_EXEC user mappings are executable.
  *
  *  *this column has no architectural effect: NX markings are ignored by
  *   hardware, but may have behavioral effects when "wants X" collides with
@@ -302,7 +303,7 @@ extern u32 elf_hwcap2;
  *
  */
 #define elf_read_implies_exec(ex, executable_stack)	\
-	(executable_stack != EXSTACK_DISABLE_X)
+	(executable_stack == EXSTACK_DEFAULT)
 
 struct task_struct;
 
-- 
2.20.1


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

* [PATCH v3 3/7] x86/elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
  2020-02-10 19:30 ` [PATCH v3 1/7] x86/elf: Add table to document READ_IMPLIES_EXEC Kees Cook
  2020-02-10 19:30 ` [PATCH v3 2/7] x86/elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-10 19:30 ` [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC Kees Cook
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

With modern x86 64-bit environments, there should never be a need for
automatic READ_IMPLIES_EXEC, as the architecture is intended to always
be execute-bit aware (as in, the default memory protection should be NX
unless a region explicitly requests to be executable).

There were very old x86_64 systems that lacked the NX bit, but for those,
the NX bit is, obviously, unenforceable, so these changes should have
no impact on them.

Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/elf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index a7035065377c..c9b7be0bcad3 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -287,7 +287,7 @@ extern u32 elf_hwcap2;
  *              CPU: | lacks NX*  | has NX, ia32     | has NX, x86_64 |
  * ELF:              |            |                  |                |
  * -------------------------------|------------------|----------------|
- * missing GNU_STACK | exec-all   | exec-all         | exec-all       |
+ * missing GNU_STACK | exec-all   | exec-all         | exec-none      |
  * GNU_STACK == RWX  | exec-stack | exec-stack       | exec-stack     |
  * GNU_STACK == RW   | exec-none  | exec-none        | exec-none      |
  *
@@ -303,7 +303,7 @@ extern u32 elf_hwcap2;
  *
  */
 #define elf_read_implies_exec(ex, executable_stack)	\
-	(executable_stack == EXSTACK_DEFAULT)
+	(mmap_is_ia32() && executable_stack == EXSTACK_DEFAULT)
 
 struct task_struct;
 
-- 
2.20.1


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

* [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
                   ` (2 preceding siblings ...)
  2020-02-10 19:30 ` [PATCH v3 3/7] x86/elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-12  9:27   ` Catalin Marinas
  2020-02-10 19:30 ` [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

Add tables to document the current behavior of READ_IMPLIES_EXEC in
preparation for changing the behavior for both arm64 and arm.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/elf.c        | 24 +++++++++++++++++++++---
 arch/arm64/include/asm/elf.h | 20 ++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index 182422981386..2f69cf978fe3 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -78,9 +78,27 @@ void elf_set_personality(const struct elf32_hdr *x)
 EXPORT_SYMBOL(elf_set_personality);
 
 /*
- * Set READ_IMPLIES_EXEC if:
- *  - the binary requires an executable stack
- *  - we're running on a CPU which doesn't support NX.
+ * An executable for which elf_read_implies_exec() returns TRUE will
+ * have the READ_IMPLIES_EXEC personality flag set automatically.
+ *
+ * The decision process for determining the results are:
+ *
+ *              CPU: | lacks NX*  | has NX     |
+ * ELF:              |            |            |
+ * -------------------------------|------------|
+ * missing GNU_STACK | exec-all   | exec-all   |
+ * GNU_STACK == RWX  | exec-all   | exec-all   |
+ * GNU_STACK == RW   | exec-all   | exec-none  |
+ *
+ *  exec-all  : all PROT_READ user mappings are executable, except when
+ *              backed by files on a noexec-filesystem.
+ *  exec-none : only PROT_EXEC user mappings are executable.
+ *
+ *  *this column has no architectural effect: NX markings are ignored by
+ *   hardware, but may have behavioral effects when "wants X" collides with
+ *   "cannot be X" constraints in memory permission flags, as in
+ *   https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
+ *
  */
 int arm_elf_read_implies_exec(int executable_stack)
 {
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index b618017205a3..7fc779e3f1ec 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -96,6 +96,26 @@
  */
 #define elf_check_arch(x)		((x)->e_machine == EM_AARCH64)
 
+/*
+ * An executable for which elf_read_implies_exec() returns TRUE will
+ * have the READ_IMPLIES_EXEC personality flag set automatically.
+ *
+ * The decision process for determining the results are:
+ *
+ *             CPU*: | arm32      | arm64      |
+ * ELF:              |            |            |
+ * -------------------------------|------------|
+ * missing GNU_STACK | exec-all   | exec-all   |
+ * GNU_STACK == RWX  | exec-all   | exec-all   |
+ * GNU_STACK == RW   | exec-none  | exec-none  |
+ *
+ *  exec-all  : all PROT_READ user mappings are executable, except when
+ *              backed by files on a noexec-filesystem.
+ *  exec-none : only PROT_EXEC user mappings are executable.
+ *
+ *  *all arm64 CPUs support NX, so there is no "lacks NX" column.
+ *
+ */
 #define elf_read_implies_exec(ex,stk)	(stk != EXSTACK_DISABLE_X)
 
 #define CORE_DUMP_USE_REGSET
-- 
2.20.1


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

* [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
                   ` (3 preceding siblings ...)
  2020-02-10 19:30 ` [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-12  9:27   ` Catalin Marinas
  2020-02-10 19:30 ` [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

The READ_IMPLIES_EXEC work-around was designed for old toolchains that
lacked the ELF PT_GNU_STACK marking under the assumption that toolchains
that couldn't specify executable permission flags for the stack may not
know how to do it correctly for any memory region.

This logic is sensible for having ancient binaries coexist in a system
with possibly NX memory, but was implemented in a way that equated having
a PT_GNU_STACK marked executable as being as "broken" as lacking the
PT_GNU_STACK marking entirely. Things like unmarked assembly and stack
trampolines may cause PT_GNU_STACK to need an executable bit, but they
do not imply all mappings must be executable.

This confusion has led to situations where modern programs with explicitly
marked executable stack are forced into the READ_IMPLIES_EXEC state when
no such thing is needed. (And leads to unexpected failures when mmap()ing
regions of device driver memory that wish to disallow VM_EXEC[1].)

In looking for other reasons for the READ_IMPLIES_EXEC behavior, Jann
Horn noted that glibc thread stacks have always been marked RWX (until
2003 when they started tracking the PT_GNU_STACK flag instead[2]). And
musl doesn't support executable stacks at all[3]. As such, no breakage
for multithreaded applications is expected from this change.

This changes arm32 and arm64 compat together, to keep behavior the same.

[1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
[2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=54ee14b3882
[3] https://lkml.kernel.org/r/20190423192534.GN23599@brightrain.aerifal.cx

Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/kernel/elf.c        | 5 +++--
 arch/arm64/include/asm/elf.h | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index 2f69cf978fe3..6965a673a141 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -87,12 +87,13 @@ EXPORT_SYMBOL(elf_set_personality);
  * ELF:              |            |            |
  * -------------------------------|------------|
  * missing GNU_STACK | exec-all   | exec-all   |
- * GNU_STACK == RWX  | exec-all   | exec-all   |
+ * GNU_STACK == RWX  | exec-all   | exec-stack |
  * GNU_STACK == RW   | exec-all   | exec-none  |
  *
  *  exec-all  : all PROT_READ user mappings are executable, except when
  *              backed by files on a noexec-filesystem.
  *  exec-none : only PROT_EXEC user mappings are executable.
+ *  exec-stack: only the stack and PROT_EXEC user mappings are executable.
  *
  *  *this column has no architectural effect: NX markings are ignored by
  *   hardware, but may have behavioral effects when "wants X" collides with
@@ -102,7 +103,7 @@ EXPORT_SYMBOL(elf_set_personality);
  */
 int arm_elf_read_implies_exec(int executable_stack)
 {
-	if (executable_stack != EXSTACK_DISABLE_X)
+	if (executable_stack == EXSTACK_DEFAULT)
 		return 1;
 	if (cpu_architecture() < CPU_ARCH_ARMv6)
 		return 1;
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 7fc779e3f1ec..03ada29984a7 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -106,17 +106,18 @@
  * ELF:              |            |            |
  * -------------------------------|------------|
  * missing GNU_STACK | exec-all   | exec-all   |
- * GNU_STACK == RWX  | exec-all   | exec-all   |
+ * GNU_STACK == RWX  | exec-stack | exec-stack |
  * GNU_STACK == RW   | exec-none  | exec-none  |
  *
  *  exec-all  : all PROT_READ user mappings are executable, except when
  *              backed by files on a noexec-filesystem.
  *  exec-none : only PROT_EXEC user mappings are executable.
+ *  exec-stack: only the stack and PROT_EXEC user mappings are executable.
  *
  *  *all arm64 CPUs support NX, so there is no "lacks NX" column.
  *
  */
-#define elf_read_implies_exec(ex,stk)	(stk != EXSTACK_DISABLE_X)
+#define elf_read_implies_exec(ex,stk)	(stk == EXSTACK_DEFAULT)
 
 #define CORE_DUMP_USE_REGSET
 #define ELF_EXEC_PAGESIZE	PAGE_SIZE
-- 
2.20.1


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

* [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
                   ` (4 preceding siblings ...)
  2020-02-10 19:30 ` [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-12  9:28   ` Catalin Marinas
  2020-02-10 19:30 ` [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests Kees Cook
  2020-02-11 17:17 ` [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Jason Gunthorpe
  7 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

With arm64 64-bit environments, there should never be a need for automatic
READ_IMPLIES_EXEC, as the architecture has always been execute-bit aware
(as in, the default memory protection should be NX unless a region
explicitly requests to be executable).

Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/include/asm/elf.h | 4 ++--
 fs/compat_binfmt_elf.c       | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 03ada29984a7..ea9221ed68a1 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -105,7 +105,7 @@
  *             CPU*: | arm32      | arm64      |
  * ELF:              |            |            |
  * -------------------------------|------------|
- * missing GNU_STACK | exec-all   | exec-all   |
+ * missing GNU_STACK | exec-all   | exec-none  |
  * GNU_STACK == RWX  | exec-stack | exec-stack |
  * GNU_STACK == RW   | exec-none  | exec-none  |
  *
@@ -117,7 +117,7 @@
  *  *all arm64 CPUs support NX, so there is no "lacks NX" column.
  *
  */
-#define elf_read_implies_exec(ex,stk)	(stk == EXSTACK_DEFAULT)
+#define compat_elf_read_implies_exec(ex, stk)	(stk == EXSTACK_DEFAULT)
 
 #define CORE_DUMP_USE_REGSET
 #define ELF_EXEC_PAGESIZE	PAGE_SIZE
diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c
index aaad4ca1217e..3068d57436b3 100644
--- a/fs/compat_binfmt_elf.c
+++ b/fs/compat_binfmt_elf.c
@@ -113,6 +113,11 @@
 #define	arch_setup_additional_pages compat_arch_setup_additional_pages
 #endif
 
+#ifdef	compat_elf_read_implies_exec
+#undef	elf_read_implies_exec
+#define	elf_read_implies_exec compat_elf_read_implies_exec
+#endif
+
 /*
  * Rename a few of the symbols that binfmt_elf.c will define.
  * These are all local so the names don't really matter, but it
-- 
2.20.1


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

* [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
                   ` (5 preceding siblings ...)
  2020-02-10 19:30 ` [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
@ 2020-02-10 19:30 ` Kees Cook
  2020-02-11 18:11   ` shuah
  2020-02-11 17:17 ` [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Jason Gunthorpe
  7 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-10 19:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Kees Cook, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

In order to check the matrix of possible states for handling
READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
add tests for these execution conditions.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/exec/Makefile         |  42 +++++-
 .../selftests/exec/read_implies_exec.c        | 121 ++++++++++++++++++
 .../selftests/exec/strip-gnu-stack-bits.c     |  34 +++++
 .../testing/selftests/exec/strip-gnu-stack.c  |  69 ++++++++++
 4 files changed, 265 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/exec/read_implies_exec.c
 create mode 100644 tools/testing/selftests/exec/strip-gnu-stack-bits.c
 create mode 100644 tools/testing/selftests/exec/strip-gnu-stack.c

diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index 33339e31e365..085d0e4422ea 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -10,7 +10,19 @@ TEST_FILES := Makefile
 
 TEST_GEN_PROGS += recursion-depth
 
-EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx*
+TEST_GEN_FILES += strip-gnu-stack
+TEST_GEN_PROGS += rie-nx-gnu-stack rie-x-gnu-stack rie-missing-gnu-stack
+
+# While it would be nice to not build "compat" binaries on 32-bit builders,
+# there's no harm: they're just redundant to the native binaries, so skip
+# performing any detection for now, as it gets complex quickly.
+TEST_GEN_PROGS += rie-compat-nx-gnu-stack \
+		  rie-compat-x-gnu-stack \
+		  rie-compat-missing-gnu-stack
+
+EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved \
+		$(OUTPUT)/rie-*.new \
+		$(OUTPUT)/xxxxx*
 
 include ../lib.mk
 
@@ -26,3 +38,31 @@ $(OUTPUT)/execveat.denatured: $(OUTPUT)/execveat
 	cp $< $@
 	chmod -x $@
 
+$(OUTPUT)/strip-gnu-stack: strip-gnu-stack.c strip-gnu-stack-bits.c
+	$(CC) $(CFLAGS) -o $@ $<
+
+$(OUTPUT)/rie-nx-gnu-stack: read_implies_exec.c
+	$(CC) $(CFLAGS) -Wl,-z,noexecstack -o $@.new $<
+	readelf -Wl $@.new | grep GNU_STACK | grep -q 'RW ' && \
+	mv $@.new $@
+$(OUTPUT)/rie-x-gnu-stack: read_implies_exec.c
+	$(CC) $(CFLAGS) -Wl,-z,execstack -o $@.new $<
+	readelf -Wl $@.new | grep GNU_STACK | grep -q 'RWE' && \
+	mv $@.new $@
+$(OUTPUT)/rie-missing-gnu-stack: read_implies_exec.c $(OUTPUT)/strip-gnu-stack
+	$(CC) $(CFLAGS) -o $@.new $<
+	$(OUTPUT)/strip-gnu-stack $@.new && \
+	mv $@.new $@
+
+$(OUTPUT)/rie-compat-nx-gnu-stack: read_implies_exec.c
+	$(CC) -m32 $(CFLAGS) -Wl,-z,noexecstack -o $@.new $<
+	readelf -Wl $@.new | grep GNU_STACK | grep -q 'RW ' && \
+	mv $@.new $@
+$(OUTPUT)/rie-compat-x-gnu-stack: read_implies_exec.c
+	$(CC) -m32 $(CFLAGS) -Wl,-z,execstack -o $@.new $<
+	readelf -Wl $@.new | grep GNU_STACK | grep -q 'RWE' && \
+	mv $@.new $@
+$(OUTPUT)/rie-compat-missing-gnu-stack: read_implies_exec.c $(OUTPUT)/strip-gnu-stack
+	$(CC) -m32 $(CFLAGS) -o $@.new $<
+	$(OUTPUT)/strip-gnu-stack $@.new && \
+	mv $@.new $@
diff --git a/tools/testing/selftests/exec/read_implies_exec.c b/tools/testing/selftests/exec/read_implies_exec.c
new file mode 100644
index 000000000000..4b253a84dd27
--- /dev/null
+++ b/tools/testing/selftests/exec/read_implies_exec.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * This just examines a PROT_READ mapping to report if it see it gain
+ * PROT_EXEC too (which means that READ_IMPLIES_EXEC has been enabled).
+ */
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+
+const char maps_path[] = "/proc/self/maps";
+
+int main(int argc, char *argv[])
+{
+	char maps_line[1024];
+	FILE *maps;
+	void *region;
+	int flags = MAP_PRIVATE | MAP_ANONYMOUS;
+	int ret = -1;
+	int perms = -1;
+	int vma_64bit;
+
+	region = mmap(NULL, getpagesize(), PROT_READ, flags, -1, 0);
+	if (region == MAP_FAILED) {
+		perror("mmap");
+		return 128;
+	}
+	maps = fopen(maps_path, "r");
+	if (!maps) {
+		perror(maps_path);
+		ret = 127;
+		goto out_munmap;
+	}
+
+	memset(maps_line, 0, sizeof(maps_line));
+	while (fgets(maps_line, sizeof(maps_line), maps)) {
+		unsigned long long low, high;
+		char *end;
+
+		low = strtoull(maps_line, &end, 16);
+		if (*end != '-') {
+			fprintf(stderr, "Missing '-' separator, line: %s",
+				maps_line);
+			ret = 126;
+			goto out_close;
+		}
+		end++;
+
+		high = strtoull(end, &end, 16);
+		if (*end != ' ') {
+			fprintf(stderr, "Missing ' ' separator, line: %s",
+				maps_line);
+			ret = 125;
+			goto out_close;
+		}
+		end++;
+
+		if ((uintptr_t)region >= low && (uintptr_t)region < high) {
+			perms = 0;
+			perms |= end[0] == 'r' ? PROT_READ : 0;
+			perms |= end[1] == 'w' ? PROT_WRITE : 0;
+			perms |= end[2] == 'x' ? PROT_EXEC : 0;
+
+			break;
+		}
+	}
+	if (perms == -1) {
+		fprintf(stderr, "Could not find mmap region\n");
+		ret = 124;
+		goto out_close;
+	}
+
+	vma_64bit = sizeof(void *) == 8;
+	fprintf(stderr, "%s-bit, ", vma_64bit ? "64" : "32");
+
+	ret = 1;
+	if (strstr(argv[0], "missing-gnu-stack")) {
+		fprintf(stderr, "missing-gnu-stack, ");
+
+		/* Missing PT_GNU_STACK on 64-bit: not READ_IMPLIES_EXEC */
+		if (vma_64bit && (perms & PROT_EXEC) == 0)
+			ret = 0;
+		/* Missing PT_GNU_STACK on 32-bit enables READ_IMPLIES_EXEC */
+		if (!vma_64bit && (perms & PROT_EXEC) == PROT_EXEC)
+			ret = 0;
+	} else if (strstr(argv[0], "x-gnu-stack")) {
+		fprintf(stderr, "executable gnu-stack, ");
+
+		/* X PT_GNU_STACK should always leave READ_IMPLIES_EXEC off */
+		if ((perms & PROT_EXEC) == 0)
+			ret = 0;
+	} else if (strstr(argv[0], "nx-gnu-stack")) {
+		fprintf(stderr, "non-executable PT_GNU_STACK, ");
+
+		/* NX PT_GNU_STACK should always leave READ_IMPLIES_EXEC off */
+		if ((perms & PROT_EXEC) == 0)
+			ret = 0;
+	} else {
+		fprintf(stderr, "Unknown invocation\n");
+		ret = 123;
+		goto out_close;
+	}
+
+	fprintf(stderr, "READ_IMPLIES_EXEC is %s: ",
+		(perms & PROT_EXEC) ? "on" : "off");
+
+	if (ret)
+		fprintf(stderr, "FAIL: %s", maps_line);
+	else
+		fprintf(stderr, "ok\n");
+
+out_close:
+	fclose(maps);
+out_munmap:
+	munmap(region, getpagesize());
+
+	return ret;
+}
diff --git a/tools/testing/selftests/exec/strip-gnu-stack-bits.c b/tools/testing/selftests/exec/strip-gnu-stack-bits.c
new file mode 100644
index 000000000000..907e959c3477
--- /dev/null
+++ b/tools/testing/selftests/exec/strip-gnu-stack-bits.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * word-size agnostic routines to scan ELF program headers for PT_GNU_STACK
+ * and rewrite it as PT_NULL to emulate old toolchains that did not include
+ * the PT_GNU_STACK program header.
+ */
+
+int strip_bits(char *elf, size_t size)
+{
+	unsigned int i;
+	Elf_Ehdr *eh;
+
+	eh = (Elf_Ehdr *)elf;
+	if (sizeof(*eh) > size) {
+		fprintf(stderr, "Elf Header too small\n");
+		return 124;
+	}
+
+	for (i = 0; i < eh->e_phnum; i++) {
+		Elf_Phdr *ph = (Elf_Phdr *)(elf + (eh->e_phoff + eh->e_phentsize * i));
+
+		if (ph->p_type == PT_GNU_STACK) {
+			ph->p_type = PT_NULL;
+			return 0;
+		}
+	}
+
+	fprintf(stderr, "PT_GNU_STACK missing\n");
+	return 123;
+}
+
+#undef strip_bits
+#undef Elf_Ehdr
+#undef Elf_Phdr
diff --git a/tools/testing/selftests/exec/strip-gnu-stack.c b/tools/testing/selftests/exec/strip-gnu-stack.c
new file mode 100644
index 000000000000..529e60cf0e6e
--- /dev/null
+++ b/tools/testing/selftests/exec/strip-gnu-stack.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Converts an ELF's PT_GNU_STACK program header to PT_NULL. */
+#include <elf.h>
+#include <fcntl.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#define strip_bits	strip64
+#define Elf_Ehdr	Elf64_Ehdr
+#define Elf_Phdr	Elf64_Phdr
+#include "strip-gnu-stack-bits.c"
+
+#define strip_bits	strip32
+#define Elf_Ehdr	Elf32_Ehdr
+#define Elf_Phdr	Elf32_Phdr
+#include "strip-gnu-stack-bits.c"
+
+int strip(char *elf, size_t size)
+{
+	if (size < 4 || elf[0] != '\x7f' || strncmp(elf + 1, "ELF", 3) != 0) {
+		fprintf(stderr, "Not an ELF file\n");
+		return 128;
+	}
+	switch (elf[EI_CLASS]) {
+	case ELFCLASS64:
+		return strip64(elf, size);
+	case ELFCLASS32:
+		return strip32(elf, size);
+	default:
+		fprintf(stderr, "Unknown EI_CLASS: 0x%02x\n", elf[EI_CLASS]);
+		return 127;
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	int fd, ret;
+	struct stat info;
+	char *elf;
+
+	fd = open(argv[1], O_RDWR);
+	if (fd < 0) {
+		perror(argv[1]);
+		return 1;
+	}
+
+	if (fstat(fd, &info)) {
+		perror(argv[1]);
+		return 2;
+	}
+
+	elf = mmap(NULL, info.st_size, PROT_READ | PROT_WRITE, MAP_SHARED,
+		   fd, 0);
+	if (elf == MAP_FAILED) {
+		perror(argv[1]);
+		return 3;
+	}
+
+	ret = strip(elf, info.st_size);
+
+	munmap(elf, info.st_size);
+	close(fd);
+	return ret;
+}
-- 
2.20.1


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

* Re: [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs
  2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
                   ` (6 preceding siblings ...)
  2020-02-10 19:30 ` [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests Kees Cook
@ 2020-02-11 17:17 ` Jason Gunthorpe
  7 siblings, 0 replies; 17+ messages in thread
From: Jason Gunthorpe @ 2020-02-11 17:17 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jann Horn, Russell King, x86, kernel-hardening, linux-arm-kernel,
	linux-kernel, linux-kselftest

On Mon, Feb 10, 2020 at 11:30:42AM -0800, Kees Cook wrote:
> Hi,
> 
> This is a refresh of my earlier attempt to fix READ_IMPLIES_EXEC. I think
> it incorporates the feedback from v2, and I've now added a selftest. This
> series is for x86, arm, and arm64; I'd like it to go via -tip, though,
> just to keep this change together with the selftest. To that end, I'd like
> to collect Acks from the respective architecture maintainers. (Note that
> most other architectures don't suffer from this problem. e.g. powerpc's
> behavior appears to already be correct. MIPS may need adjusting but the
> history of CPU features and toolchain behavior is very unclear to me.)
> 
> Repeating the commit log from later in the series:
> 
> 
> The READ_IMPLIES_EXEC work-around was designed for old toolchains that
> lacked the ELF PT_GNU_STACK marking under the assumption that toolchains
> that couldn't specify executable permission flags for the stack may not
> know how to do it correctly for any memory region.
> 
> This logic is sensible for having ancient binaries coexist in a system
> with possibly NX memory, but was implemented in a way that equated having
> a PT_GNU_STACK marked executable as being as "broken" as lacking the
> PT_GNU_STACK marking entirely. Things like unmarked assembly and stack
> trampolines may cause PT_GNU_STACK to need an executable bit, but they
> do not imply all mappings must be executable.
> 
> This confusion has led to situations where modern programs with explicitly
> marked executable stack are forced into the READ_IMPLIES_EXEC state when
> no such thing is needed. (And leads to unexpected failures when mmap()ing
> regions of device driver memory that wish to disallow VM_EXEC[1].)
> 
> In looking for other reasons for the READ_IMPLIES_EXEC behavior, Jann
> Horn noted that glibc thread stacks have always been marked RWX (until
> 2003 when they started tracking the PT_GNU_STACK flag instead[2]). And
> musl doesn't support executable stacks at all[3]. As such, no breakage
> for multithreaded applications is expected from this change.
> 
> [1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
> [2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=54ee14b3882
> [3] https://lkml.kernel.org/r/20190423192534.GN23599@brightrain.aerifal.cx

I'm happy to see this, I think it will help the situation.

While I'm not well versed in all the historical details, the general
approach makes sense to me and I've looked through the patches.

I would like to follow this up with a patch to again block VM_EXEC
from the RDMA related mmap of BAR paths..

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason

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

* Re: [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-10 19:30 ` [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests Kees Cook
@ 2020-02-11 18:11   ` shuah
  2020-02-11 19:25     ` Kees Cook
  0 siblings, 1 reply; 17+ messages in thread
From: shuah @ 2020-02-11 18:11 UTC (permalink / raw)
  To: Kees Cook, Ingo Molnar
  Cc: Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest, shuah

On 2/10/20 12:30 PM, Kees Cook wrote:
> In order to check the matrix of possible states for handling
> READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
> add tests for these execution conditions.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

No issues for this to go through tip.

A few problems to fix first. This fails to compile when 32-bit libraries
aren't installed. It should fail the 32-bit part and run other checks.

make kselftest TARGETS=exec
make --no-builtin-rules ARCH=x86 -C ../../.. headers_install
make[2]: Entering directory '/lkml/linux_5.6'
   INSTALL ./usr/include
make[2]: Leaving directory '/lkml/linux_5.6'
make[2]: Entering directory '/lkml/linux_5.6/tools/testing/selftests/exec'
gcc -m32 -Wall -Wno-nonnull -D_GNU_SOURCE -Wl,-z,noexecstack -o 
/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack.new 
read_implies_exec.c
readelf -Wl 
/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack.new 
| grep GNU_STACK | grep -q 'RW ' && \
mv 
/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack.new 
/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack
In file included from /usr/lib/gcc/x86_64-linux-gnu/9/include/stdint.h:9,
                  from read_implies_exec.c:6:
/usr/include/stdint.h:26:10: fatal error: bits/libc-header-start.h: No 
such file or directory
    26 | #include <bits/libc-header-start.h>
       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
readelf: Error: 
'/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack.new': 
No such file
make[2]: *** [Makefile:58: 
/lkml/linux_5.6/tools/testing/selftests/exec/rie-compat-nx-gnu-stack] 
Error 1
make[2]: Leaving directory '/lkml/linux_5.6/tools/testing/selftests/exec'
make[1]: *** [Makefile:150: all] Error 2
make: *** [Makefile:1217: kselftest] Error 2

thanks,
-- Shuah

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

* Re: [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-11 18:11   ` shuah
@ 2020-02-11 19:25     ` Kees Cook
  2020-02-11 21:06       ` shuah
  0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-11 19:25 UTC (permalink / raw)
  To: shuah
  Cc: Ingo Molnar, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

On Tue, Feb 11, 2020 at 11:11:21AM -0700, shuah wrote:
> On 2/10/20 12:30 PM, Kees Cook wrote:
> > In order to check the matrix of possible states for handling
> > READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
> > add tests for these execution conditions.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> No issues for this to go through tip.
> 
> A few problems to fix first. This fails to compile when 32-bit libraries
> aren't installed. It should fail the 32-bit part and run other checks.

Do you mean the Makefile should detect the missing compat build deps and
avoid building them? Testing compat is pretty important to this test, so
it seems like missing the build deps causing the build to fail is the
correct action here. This is likely true for the x86/ selftests too.

What would you like this to do?

-- 
Kees Cook

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

* Re: [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-11 19:25     ` Kees Cook
@ 2020-02-11 21:06       ` shuah
  2020-02-11 23:54         ` Kees Cook
  0 siblings, 1 reply; 17+ messages in thread
From: shuah @ 2020-02-11 21:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest, shuah

On 2/11/20 12:25 PM, Kees Cook wrote:
> On Tue, Feb 11, 2020 at 11:11:21AM -0700, shuah wrote:
>> On 2/10/20 12:30 PM, Kees Cook wrote:
>>> In order to check the matrix of possible states for handling
>>> READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
>>> add tests for these execution conditions.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> No issues for this to go through tip.
>>
>> A few problems to fix first. This fails to compile when 32-bit libraries
>> aren't installed. It should fail the 32-bit part and run other checks.
> 
> Do you mean the Makefile should detect the missing compat build deps and
> avoid building them? Testing compat is pretty important to this test, so
> it seems like missing the build deps causing the build to fail is the
> correct action here. This is likely true for the x86/ selftests too.
> 
> What would you like this to do?
> 

selftests/x86 does this already and runs the dependency check in 
x86/Makefile.


check_cc.sh:# check_cc.sh - Helper to test userspace compilation support
Makefile:CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC) 
trivial_32bit_program.c -m32)
Makefile:CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC) 
trivial_64bit_program.c)
Makefile:CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) 
trivial_program.c -no-pie)

Take a look and see if you can leverage this.

thanks,
-- Shuah

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

* Re: [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-11 21:06       ` shuah
@ 2020-02-11 23:54         ` Kees Cook
  2020-02-12  0:02           ` shuah
  0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2020-02-11 23:54 UTC (permalink / raw)
  To: shuah
  Cc: Ingo Molnar, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest

On Tue, Feb 11, 2020 at 02:06:53PM -0700, shuah wrote:
> On 2/11/20 12:25 PM, Kees Cook wrote:
> > On Tue, Feb 11, 2020 at 11:11:21AM -0700, shuah wrote:
> > > On 2/10/20 12:30 PM, Kees Cook wrote:
> > > > In order to check the matrix of possible states for handling
> > > > READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
> > > > add tests for these execution conditions.
> > > > 
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > 
> > > No issues for this to go through tip.
> > > 
> > > A few problems to fix first. This fails to compile when 32-bit libraries
> > > aren't installed. It should fail the 32-bit part and run other checks.
> > 
> > Do you mean the Makefile should detect the missing compat build deps and
> > avoid building them? Testing compat is pretty important to this test, so
> > it seems like missing the build deps causing the build to fail is the
> > correct action here. This is likely true for the x86/ selftests too.
> > 
> > What would you like this to do?
> > 
> 
> selftests/x86 does this already and runs the dependency check in
> x86/Makefile.
> 
> 
> check_cc.sh:# check_cc.sh - Helper to test userspace compilation support
> Makefile:CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC)
> trivial_32bit_program.c -m32)
> Makefile:CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC)
> trivial_64bit_program.c)
> Makefile:CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC)
> trivial_program.c -no-pie)
> 
> Take a look and see if you can leverage this.

I did before, and it can certainly be done, but their stuff is somewhat
specific to x86_64/ia32. I'm looking at supporting _all_ compat for any
64-bit architecture. I can certainly write some similar build tooling,
but the question I have for you is one of coverage:

If a builder is 64-bit, it needs to be able to produce 32-bit compat
binaries for testing, otherwise the test is incomplete. (i.e. the tests
will only be able to test native behavior and not compat). This doesn't
seem like an "XFAIL" situation to me, and it doesn't seem right to
silently pass. It seems like the build should explicitly fail because
the needed prerequisites are missing. Do you instead want me to just
have it skip building the compat binaries if it can't build them?

-- 
Kees Cook

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

* Re: [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests
  2020-02-11 23:54         ` Kees Cook
@ 2020-02-12  0:02           ` shuah
  0 siblings, 0 replies; 17+ messages in thread
From: shuah @ 2020-02-12  0:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Catalin Marinas, Will Deacon,
	Jason Gunthorpe, Jann Horn, Russell King, x86, kernel-hardening,
	linux-arm-kernel, linux-kernel, linux-kselftest, shuah

On 2/11/20 4:54 PM, Kees Cook wrote:
> On Tue, Feb 11, 2020 at 02:06:53PM -0700, shuah wrote:
>> On 2/11/20 12:25 PM, Kees Cook wrote:
>>> On Tue, Feb 11, 2020 at 11:11:21AM -0700, shuah wrote:
>>>> On 2/10/20 12:30 PM, Kees Cook wrote:
>>>>> In order to check the matrix of possible states for handling
>>>>> READ_IMPLIES_EXEC across native, compat, and the state of PT_GNU_STACK,
>>>>> add tests for these execution conditions.
>>>>>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>
>>>> No issues for this to go through tip.
>>>>
>>>> A few problems to fix first. This fails to compile when 32-bit libraries
>>>> aren't installed. It should fail the 32-bit part and run other checks.
>>>
>>> Do you mean the Makefile should detect the missing compat build deps and
>>> avoid building them? Testing compat is pretty important to this test, so
>>> it seems like missing the build deps causing the build to fail is the
>>> correct action here. This is likely true for the x86/ selftests too.
>>>
>>> What would you like this to do?
>>>
>>
>> selftests/x86 does this already and runs the dependency check in
>> x86/Makefile.
>>
>>
>> check_cc.sh:# check_cc.sh - Helper to test userspace compilation support
>> Makefile:CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC)
>> trivial_32bit_program.c -m32)
>> Makefile:CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC)
>> trivial_64bit_program.c)
>> Makefile:CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC)
>> trivial_program.c -no-pie)
>>
>> Take a look and see if you can leverage this.
> 
> I did before, and it can certainly be done, but their stuff is somewhat
> specific to x86_64/ia32. I'm looking at supporting _all_ compat for any
> 64-bit architecture. I can certainly write some similar build tooling,
> but the question I have for you is one of coverage:
> 
> If a builder is 64-bit, it needs to be able to produce 32-bit compat
> binaries for testing, otherwise the test is incomplete. (i.e. the tests
> will only be able to test native behavior and not compat). This doesn't
> seem like an "XFAIL" situation to me, and it doesn't seem right to
> silently pass. It seems like the build should explicitly fail because
> the needed prerequisites are missing. Do you instead want me to just
> have it skip building the compat binaries if it can't build them?
> 

Can we do the following:


Build and run tests thatc an be built.
Skip build and warn that test coverage is incomplete for compat
with a strong recommendation on installing 32-bit libraries with
some instructions on how to if applicable.

thanks,
-- Shuah

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

* Re: [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC
  2020-02-10 19:30 ` [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC Kees Cook
@ 2020-02-12  9:27   ` Catalin Marinas
  0 siblings, 0 replies; 17+ messages in thread
From: Catalin Marinas @ 2020-02-12  9:27 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Will Deacon, Jason Gunthorpe,
	Jann Horn, Russell King, x86, kernel-hardening, linux-arm-kernel,
	linux-kernel, linux-kselftest

On Mon, Feb 10, 2020 at 11:30:46AM -0800, Kees Cook wrote:
> Add tables to document the current behavior of READ_IMPLIES_EXEC in
> preparation for changing the behavior for both arm64 and arm.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

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

* Re: [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK
  2020-02-10 19:30 ` [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
@ 2020-02-12  9:27   ` Catalin Marinas
  0 siblings, 0 replies; 17+ messages in thread
From: Catalin Marinas @ 2020-02-12  9:27 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Will Deacon, Jason Gunthorpe,
	Jann Horn, Russell King, x86, kernel-hardening, linux-arm-kernel,
	linux-kernel, linux-kselftest

On Mon, Feb 10, 2020 at 11:30:47AM -0800, Kees Cook wrote:
> The READ_IMPLIES_EXEC work-around was designed for old toolchains that
> lacked the ELF PT_GNU_STACK marking under the assumption that toolchains
> that couldn't specify executable permission flags for the stack may not
> know how to do it correctly for any memory region.
> 
> This logic is sensible for having ancient binaries coexist in a system
> with possibly NX memory, but was implemented in a way that equated having
> a PT_GNU_STACK marked executable as being as "broken" as lacking the
> PT_GNU_STACK marking entirely. Things like unmarked assembly and stack
> trampolines may cause PT_GNU_STACK to need an executable bit, but they
> do not imply all mappings must be executable.
> 
> This confusion has led to situations where modern programs with explicitly
> marked executable stack are forced into the READ_IMPLIES_EXEC state when
> no such thing is needed. (And leads to unexpected failures when mmap()ing
> regions of device driver memory that wish to disallow VM_EXEC[1].)
> 
> In looking for other reasons for the READ_IMPLIES_EXEC behavior, Jann
> Horn noted that glibc thread stacks have always been marked RWX (until
> 2003 when they started tracking the PT_GNU_STACK flag instead[2]). And
> musl doesn't support executable stacks at all[3]. As such, no breakage
> for multithreaded applications is expected from this change.
> 
> This changes arm32 and arm64 compat together, to keep behavior the same.
> 
> [1] https://lkml.kernel.org/r/20190418055759.GA3155@mellanox.com
> [2] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=54ee14b3882
> [3] https://lkml.kernel.org/r/20190423192534.GN23599@brightrain.aerifal.cx
> 
> Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

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

* Re: [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces
  2020-02-10 19:30 ` [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
@ 2020-02-12  9:28   ` Catalin Marinas
  0 siblings, 0 replies; 17+ messages in thread
From: Catalin Marinas @ 2020-02-12  9:28 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Hector Marco-Gisbert, Will Deacon, Jason Gunthorpe,
	Jann Horn, Russell King, x86, kernel-hardening, linux-arm-kernel,
	linux-kernel, linux-kselftest

On Mon, Feb 10, 2020 at 11:30:48AM -0800, Kees Cook wrote:
> With arm64 64-bit environments, there should never be a need for automatic
> READ_IMPLIES_EXEC, as the architecture has always been execute-bit aware
> (as in, the default memory protection should be NX unless a region
> explicitly requests to be executable).
> 
> Suggested-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

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

end of thread, other threads:[~2020-02-12  9:28 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10 19:30 [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Kees Cook
2020-02-10 19:30 ` [PATCH v3 1/7] x86/elf: Add table to document READ_IMPLIES_EXEC Kees Cook
2020-02-10 19:30 ` [PATCH v3 2/7] x86/elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
2020-02-10 19:30 ` [PATCH v3 3/7] x86/elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
2020-02-10 19:30 ` [PATCH v3 4/7] arm32/64, elf: Add tables to document READ_IMPLIES_EXEC Kees Cook
2020-02-12  9:27   ` Catalin Marinas
2020-02-10 19:30 ` [PATCH v3 5/7] arm32/64, elf: Split READ_IMPLIES_EXEC from executable GNU_STACK Kees Cook
2020-02-12  9:27   ` Catalin Marinas
2020-02-10 19:30 ` [PATCH v3 6/7] arm64, elf: Disable automatic READ_IMPLIES_EXEC for 64-bit address spaces Kees Cook
2020-02-12  9:28   ` Catalin Marinas
2020-02-10 19:30 ` [PATCH v3 7/7] selftests/exec: Add READ_IMPLIES_EXEC tests Kees Cook
2020-02-11 18:11   ` shuah
2020-02-11 19:25     ` Kees Cook
2020-02-11 21:06       ` shuah
2020-02-11 23:54         ` Kees Cook
2020-02-12  0:02           ` shuah
2020-02-11 17:17 ` [PATCH v3 0/7] binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs Jason Gunthorpe

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