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