diff mbox series

[v3,7/7] selftests/exec: Add READ_IMPLIES_EXEC tests

Message ID 20200210193049.64362-8-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series binfmt_elf: Update READ_IMPLIES_EXEC logic for modern CPUs | expand

Commit Message

Kees Cook Feb. 10, 2020, 7:30 p.m. UTC
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

Comments

Shuah Feb. 11, 2020, 6:11 p.m. UTC | #1
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
Kees Cook Feb. 11, 2020, 7:25 p.m. UTC | #2
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?
Shuah Feb. 11, 2020, 9:06 p.m. UTC | #3
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
Kees Cook Feb. 11, 2020, 11:54 p.m. UTC | #4
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?
Shuah Feb. 12, 2020, 12:02 a.m. UTC | #5
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
diff mbox series

Patch

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;
+}