| Message ID | 20240508173149.677910-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | binfmt_elf: Honor PT_LOAD alignment for static PIE | expand |
On 5/8/24 10:31 AM, Kees Cook wrote: > After commit 4d1cd3b2c5c1 ("tools/testing/selftests/exec: fix link > error"), the load address alignment tests tried to build statically. > This was silently ignored in some cases. However, after attempting to > further fix the build by switching to "-static-pie", the test started > failing. This appears to be due to non-PT_INTERP ET_DYN execs ("static > PIE") not doing alignment correctly, which remains unfixed[1]. See commit > aeb7923733d1 ("revert "fs/binfmt_elf: use PT_LOAD p_align values for > static PIE"") for more details. > > Provide rules to build both static and non-static PIE binaries, improve > debug reporting, and perform several test steps instead of a single > all-or-nothing test. However, do not actually enable static-pie tests; > alignment specification is only supported for ET_DYN with PT_INTERP > ("regular PIE"). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215275 [1] > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Cc: Chris Kennelly <ckennelly@google.com> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Shuah Khan <shuah@kernel.org> > Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Fangrui Song <maskray@google.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Yang Yingliang <yangyingliang@huawei.com> > Cc: linux-mm@kvack.org > Cc: linux-kselftest@vger.kernel.org > --- > tools/testing/selftests/exec/Makefile | 19 +++--- > tools/testing/selftests/exec/load_address.c | 67 +++++++++++++++++---- > 2 files changed, 66 insertions(+), 20 deletions(-) > > diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile > index fb4472ddffd8..619cff81d796 100644 > --- a/tools/testing/selftests/exec/Makefile > +++ b/tools/testing/selftests/exec/Makefile > @@ -3,8 +3,13 @@ CFLAGS = -Wall > CFLAGS += -Wno-nonnull > CFLAGS += -D_GNU_SOURCE > > +ALIGNS := 0x1000 0x200000 0x1000000 > +ALIGN_PIES := $(patsubst %,load_address.%,$(ALIGNS)) > +ALIGN_STATIC_PIES := $(patsubst %,load_address.static.%,$(ALIGNS)) > +ALIGNMENT_TESTS := $(ALIGN_PIES) > + > TEST_PROGS := binfmt_script.py > -TEST_GEN_PROGS := execveat load_address_4096 load_address_2097152 load_address_16777216 non-regular > +TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS) > TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir > # Makefile is a run-time dependency, since it's accessed by the execveat test > TEST_FILES := Makefile > @@ -28,9 +33,9 @@ $(OUTPUT)/execveat.symlink: $(OUTPUT)/execveat > $(OUTPUT)/execveat.denatured: $(OUTPUT)/execveat > cp $< $@ > chmod -x $@ > -$(OUTPUT)/load_address_4096: load_address.c > - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x1000 -pie -static $< -o $@ > -$(OUTPUT)/load_address_2097152: load_address.c > - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x200000 -pie -static $< -o $@ > -$(OUTPUT)/load_address_16777216: load_address.c > - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x1000000 -pie -static $< -o $@ > +$(OUTPUT)/load_address.0x%: load_address.c > + $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=$(lastword $(subst ., ,$@)) \ > + -fPIE -pie $< -o $@ > +$(OUTPUT)/load_address.static.0x%: load_address.c > + $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=$(lastword $(subst ., ,$@)) \ > + -fPIE -static-pie $< -o $@ Hi Kees, Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the kernel's minimum gcc version is 5? thanks,
On Wed, May 08, 2024 at 07:54:13PM -0700, John Hubbard wrote: > Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the > kernel's minimum gcc version is 5? Yes, that's true. If we encounter anyone trying to build the selftests with <8.1 I think we'll have to add a compiler version test in the Makefile to exclude the static pie tests. There's also the potential issue with arm64 builds that caused the original attempt at -static. We'll likely need an exclusion there too.
On 5/9/24 11:16 AM, Kees Cook wrote: > On Wed, May 08, 2024 at 07:54:13PM -0700, John Hubbard wrote: >> Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the >> kernel's minimum gcc version is 5? > > Yes, that's true. If we encounter anyone trying to build the selftests > with <8.1 I think we'll have to add a compiler version test in the > Makefile to exclude the static pie tests. > > There's also the potential issue with arm64 builds that caused the > original attempt at -static. We'll likely need an exclusion there too. > I'm not getting failures for arm64 instead for arm. I'm trying to find this "rcrt1.o" file. Does anybody have any idea if this error can be resolved by missing file or is it something arm-linux-gnueabihf toolchain doesn't support? make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- arm-linux-gnueabihf-gcc -Wall -Wno-nonnull -D_GNU_SOURCE= -Wl,-z,max-page-size=0x1000 \ -fPIE -static-pie load_address.c -o /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000 /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: cannot find rcrt1.o: No such file or directory collect2: error: ld returned 1 exit status make: *** [Makefile:39: /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000] Error 1
On 8/7/24 3:22 PM, Muhammad Usama Anjum wrote: > On 5/9/24 11:16 AM, Kees Cook wrote: >> On Wed, May 08, 2024 at 07:54:13PM -0700, John Hubbard wrote: >>> Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the >>> kernel's minimum gcc version is 5? >> >> Yes, that's true. If we encounter anyone trying to build the selftests >> with <8.1 I think we'll have to add a compiler version test in the >> Makefile to exclude the static pie tests. >> >> There's also the potential issue with arm64 builds that caused the >> original attempt at -static. We'll likely need an exclusion there too. >> > I'm not getting failures for arm64 instead for arm. I'm trying to find > this "rcrt1.o" file. Does anybody have any idea if this error can be > resolved by missing file or is it something arm-linux-gnueabihf > toolchain doesn't support? Do you have any idea? > > make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- > arm-linux-gnueabihf-gcc -Wall -Wno-nonnull -D_GNU_SOURCE= > -Wl,-z,max-page-size=0x1000 \ > -fPIE -static-pie load_address.c -o > /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000 > /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: > cannot find rcrt1.o: No such file or directory > collect2: error: ld returned 1 exit status > make: *** [Makefile:39: > /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000] > Error 1
On 8/18/24 8:55 PM, Muhammad Usama Anjum wrote: > On 8/7/24 3:22 PM, Muhammad Usama Anjum wrote: >> On 5/9/24 11:16 AM, Kees Cook wrote: >>> On Wed, May 08, 2024 at 07:54:13PM -0700, John Hubbard wrote: >>>> Didn't we learn recently, though, that -static-pie is gcc 8.1+, while the >>>> kernel's minimum gcc version is 5? >>> >>> Yes, that's true. If we encounter anyone trying to build the selftests >>> with <8.1 I think we'll have to add a compiler version test in the >>> Makefile to exclude the static pie tests. >>> >>> There's also the potential issue with arm64 builds that caused the >>> original attempt at -static. We'll likely need an exclusion there too. >>> >> I'm not getting failures for arm64 instead for arm. I'm trying to find >> this "rcrt1.o" file. Does anybody have any idea if this error can be >> resolved by missing file or is it something arm-linux-gnueabihf >> toolchain doesn't support? > Do you have any idea? Yes, I took a closer look just now: > >> >> make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- >> arm-linux-gnueabihf-gcc -Wall -Wno-nonnull -D_GNU_SOURCE= >> -Wl,-z,max-page-size=0x1000 \ >> -fPIE -static-pie load_address.c -o >> /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000 >> /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: >> cannot find rcrt1.o: No such file or directory >> collect2: error: ld returned 1 exit status >> make: *** [Makefile:39: >> /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000] >> Error 1 > This appears to be because that particular cross compiler setup (the libc part of the installation) fails to include rcrt1.o. I was able to reproduce this on Ubuntu 23.04, using their standard arm cross compilation packages for gcc and libc. Also, -static-pie is what is causing the linker to look for rcrt1.o. If you change the invocation from "-static-pie" to "-static -pie", then linking succeeds. Putting all of this together, I think we were are seeing is that even though "-static-pie" was added to gcc 8.1+, and even though the cross-compiler installation here shows gcc 12.3.0, the libc support for that feature is lagging. In other words, the cross installation of libc is effectively at something earlier than gcc 8.1's needs. So I think this means that we need to fix up the distros'packages, and meanwhile, fall back to something like "-static-pie" for selftests. thanks,
On 8/19/24 12:41 PM, John Hubbard wrote: > On 8/18/24 8:55 PM, Muhammad Usama Anjum wrote: >> On 8/7/24 3:22 PM, Muhammad Usama Anjum wrote: >>> On 5/9/24 11:16 AM, Kees Cook wrote: ... >>> make ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- >>> arm-linux-gnueabihf-gcc -Wall -Wno-nonnull -D_GNU_SOURCE= >>> -Wl,-z,max-page-size=0x1000 \ >>> -fPIE -static-pie load_address.c -o >>> /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000 >>> /usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: >>> cannot find rcrt1.o: No such file or directory >>> collect2: error: ld returned 1 exit status >>> make: *** [Makefile:39: >>> /home/usama/repos/kernel/linux_mainline/tools/testing/selftests/exec/load_address.static.0x1000] >>> Error 1 > > This appears to be because that particular cross compiler setup > (the libc part of the installation) fails to include rcrt1.o. I was > able to reproduce this on Ubuntu 23.04, using their standard arm > cross compilation packages for gcc and libc. > > Also, -static-pie is what is causing the linker to look for rcrt1.o. > If you change the invocation from "-static-pie" to "-static -pie", > then linking succeeds. > > Putting all of this together, I think we were are seeing is that > even though "-static-pie" was added to gcc 8.1+, and even though > the cross-compiler installation here shows gcc 12.3.0, the libc > support for that feature is lagging. In other words, the cross > installation of libc is effectively at something earlier than > gcc 8.1's needs. > > So I think this means that we need to fix up the distros'packages, > and meanwhile, fall back to something like "-static-pie" for > selftests. > Or, another way of handling this would be to say, "gcc 8.1 is sufficiently old, and cross-compiling of kselftests is sufficiently rare, that we can wait for the cross compilers to catch up. Until then, only native builds of kselftests are supported". Shuah, you would know whether or not that's a reasonable thing to claim--what do you think? I'm just trying to list all of the options so that people can decide which way to go here. thanks,
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile index fb4472ddffd8..619cff81d796 100644 --- a/tools/testing/selftests/exec/Makefile +++ b/tools/testing/selftests/exec/Makefile @@ -3,8 +3,13 @@ CFLAGS = -Wall CFLAGS += -Wno-nonnull CFLAGS += -D_GNU_SOURCE +ALIGNS := 0x1000 0x200000 0x1000000 +ALIGN_PIES := $(patsubst %,load_address.%,$(ALIGNS)) +ALIGN_STATIC_PIES := $(patsubst %,load_address.static.%,$(ALIGNS)) +ALIGNMENT_TESTS := $(ALIGN_PIES) + TEST_PROGS := binfmt_script.py -TEST_GEN_PROGS := execveat load_address_4096 load_address_2097152 load_address_16777216 non-regular +TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS) TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir # Makefile is a run-time dependency, since it's accessed by the execveat test TEST_FILES := Makefile @@ -28,9 +33,9 @@ $(OUTPUT)/execveat.symlink: $(OUTPUT)/execveat $(OUTPUT)/execveat.denatured: $(OUTPUT)/execveat cp $< $@ chmod -x $@ -$(OUTPUT)/load_address_4096: load_address.c - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x1000 -pie -static $< -o $@ -$(OUTPUT)/load_address_2097152: load_address.c - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x200000 -pie -static $< -o $@ -$(OUTPUT)/load_address_16777216: load_address.c - $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=0x1000000 -pie -static $< -o $@ +$(OUTPUT)/load_address.0x%: load_address.c + $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=$(lastword $(subst ., ,$@)) \ + -fPIE -pie $< -o $@ +$(OUTPUT)/load_address.static.0x%: load_address.c + $(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=$(lastword $(subst ., ,$@)) \ + -fPIE -static-pie $< -o $@ diff --git a/tools/testing/selftests/exec/load_address.c b/tools/testing/selftests/exec/load_address.c index 17e3207d34ae..8257fddba8c8 100644 --- a/tools/testing/selftests/exec/load_address.c +++ b/tools/testing/selftests/exec/load_address.c @@ -5,11 +5,13 @@ #include <link.h> #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include "../kselftest.h" struct Statistics { unsigned long long load_address; unsigned long long alignment; + bool interp; }; int ExtractStatistics(struct dl_phdr_info *info, size_t size, void *data) @@ -26,11 +28,20 @@ int ExtractStatistics(struct dl_phdr_info *info, size_t size, void *data) stats->alignment = 0; for (i = 0; i < info->dlpi_phnum; i++) { + unsigned long long align; + + if (info->dlpi_phdr[i].p_type == PT_INTERP) { + stats->interp = true; + continue; + } + if (info->dlpi_phdr[i].p_type != PT_LOAD) continue; - if (info->dlpi_phdr[i].p_align > stats->alignment) - stats->alignment = info->dlpi_phdr[i].p_align; + align = info->dlpi_phdr[i].p_align; + + if (align > stats->alignment) + stats->alignment = align; } return 1; // Terminate dl_iterate_phdr. @@ -38,27 +49,57 @@ int ExtractStatistics(struct dl_phdr_info *info, size_t size, void *data) int main(int argc, char **argv) { - struct Statistics extracted; - unsigned long long misalign; + struct Statistics extracted = { }; + unsigned long long misalign, pow2; + bool interp_needed; + char buf[1024]; + FILE *maps; int ret; ksft_print_header(); - ksft_set_plan(1); + ksft_set_plan(4); + + /* Dump maps file for debugging reference. */ + maps = fopen("/proc/self/maps", "r"); + if (!maps) + ksft_exit_fail_msg("FAILED: /proc/self/maps: %s\n", strerror(errno)); + while (fgets(buf, sizeof(buf), maps)) { + ksft_print_msg("%s", buf); + } + fclose(maps); + /* Walk the program headers. */ ret = dl_iterate_phdr(ExtractStatistics, &extracted); if (ret != 1) ksft_exit_fail_msg("FAILED: dl_iterate_phdr\n"); - if (extracted.alignment == 0) - ksft_exit_fail_msg("FAILED: No alignment found\n"); - else if (extracted.alignment & (extracted.alignment - 1)) - ksft_exit_fail_msg("FAILED: Alignment is not a power of 2\n"); + /* Report our findings. */ + ksft_print_msg("load_address=%#llx alignment=%#llx\n", + extracted.load_address, extracted.alignment); + + /* If we're named with ".static." we expect no INTERP. */ + interp_needed = strstr(argv[0], ".static.") == NULL; + + /* Were we built as expected? */ + ksft_test_result(interp_needed == extracted.interp, + "%s INTERP program header %s\n", + interp_needed ? "Wanted" : "Unwanted", + extracted.interp ? "seen" : "missing"); + + /* Did we find an alignment? */ + ksft_test_result(extracted.alignment != 0, + "Alignment%s found\n", extracted.alignment ? "" : " NOT"); + + /* Is the alignment sane? */ + pow2 = extracted.alignment & (extracted.alignment - 1); + ksft_test_result(pow2 == 0, + "Alignment is%s a power of 2: %#llx\n", + pow2 == 0 ? "" : " NOT", extracted.alignment); + /* Is the load address aligned? */ misalign = extracted.load_address & (extracted.alignment - 1); - if (misalign) - ksft_exit_fail_msg("FAILED: alignment = %llu, load_address = %llu\n", - extracted.alignment, extracted.load_address); + ksft_test_result(misalign == 0, "Load Address is %saligned (%#llx)\n", + misalign ? "MIS" : "", misalign); - ksft_test_result_pass("Completed\n"); ksft_finished(); }
After commit 4d1cd3b2c5c1 ("tools/testing/selftests/exec: fix link error"), the load address alignment tests tried to build statically. This was silently ignored in some cases. However, after attempting to further fix the build by switching to "-static-pie", the test started failing. This appears to be due to non-PT_INTERP ET_DYN execs ("static PIE") not doing alignment correctly, which remains unfixed[1]. See commit aeb7923733d1 ("revert "fs/binfmt_elf: use PT_LOAD p_align values for static PIE"") for more details. Provide rules to build both static and non-static PIE binaries, improve debug reporting, and perform several test steps instead of a single all-or-nothing test. However, do not actually enable static-pie tests; alignment specification is only supported for ET_DYN with PT_INTERP ("regular PIE"). Link: https://bugzilla.kernel.org/show_bug.cgi?id=215275 [1] Signed-off-by: Kees Cook <keescook@chromium.org> --- Cc: Chris Kennelly <ckennelly@google.com> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Shuah Khan <shuah@kernel.org> Cc: Muhammad Usama Anjum <usama.anjum@collabora.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Fangrui Song <maskray@google.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Yang Yingliang <yangyingliang@huawei.com> Cc: linux-mm@kvack.org Cc: linux-kselftest@vger.kernel.org --- tools/testing/selftests/exec/Makefile | 19 +++--- tools/testing/selftests/exec/load_address.c | 67 +++++++++++++++++---- 2 files changed, 66 insertions(+), 20 deletions(-)