[PATCH 0/3] binfmt_elf: Honor PT_LOAD alignment for static PIE

[PATCH 0/3] binfmt_elf: Honor PT_LOAD alignment for static PIE

[Kees Cook]

Hi,

This attempts to implement PT_LOAD p_align support for static PIE builds.
I intend this to go into -next after the coming merge window so we can
maximize bake time. In the past we've had regressions with both the
selftests and the ELF loader. Hopefully we can shake everything out over
a few months. :)

Thanks!

-Kees

Kees Cook (3):
  selftests/exec: Build both static and non-static load_address tests
  binfmt_elf: Calculate total_size earlier
  binfmt_elf: Honor PT_LOAD alignment for static PIE

 fs/binfmt_elf.c                             | 94 ++++++++++++++-------
 tools/testing/selftests/exec/Makefile       | 19 +++--
 tools/testing/selftests/exec/load_address.c | 67 ++++++++++++---
 3 files changed, 130 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[Kees Cook]

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 $@
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();
 }
-- 
2.34.1

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

> 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();
>   }

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[Muhammad Usama Anjum]

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

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[Muhammad Usama Anjum]

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


-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA
  

Re: [PATCH 1/3] selftests/exec: Build both static and non-static load_address tests

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

[PATCH 2/3] binfmt_elf: Calculate total_size earlier

[Kees Cook]

In preparation to support PT_LOAD with large p_align values on
non-PT_INTERP ET_DYN executables (i.e. "static pie"), we'll need to use
the total_size details earlier. Move this separately now to make the
next patch more readable. As total_size and load_bias are currently
calculated separately, this has no behavioral impact.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/binfmt_elf.c | 52 +++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5397b552fbeb..56432e019d4e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1061,7 +1061,34 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			 * Header for ET_DYN binaries to calculate the
 			 * randomization (load_bias) for all the LOAD
 			 * Program Headers.
+			 */
+
+			/*
+			 * Calculate the entire size of the ELF mapping
+			 * (total_size), used for the initial mapping,
+			 * due to load_addr_set which is set to true later
+			 * once the initial mapping is performed.
+			 *
+			 * Note that this is only sensible when the LOAD
+			 * segments are contiguous (or overlapping). If
+			 * used for LOADs that are far apart, this would
+			 * cause the holes between LOADs to be mapped,
+			 * running the risk of having the mapping fail,
+			 * as it would be larger than the ELF file itself.
 			 *
+			 * As a result, only ET_DYN does this, since
+			 * some ET_EXEC (e.g. ia64) may have large virtual
+			 * memory holes between LOADs.
+			 *
+			 */
+			total_size = total_mapping_size(elf_phdata,
+							elf_ex->e_phnum);
+			if (!total_size) {
+				retval = -EINVAL;
+				goto out_free_dentry;
+			}
+
+			/*
 			 * There are effectively two types of ET_DYN
 			 * binaries: programs (i.e. PIE: ET_DYN with INTERP)
 			 * and loaders (ET_DYN without INTERP, since they
@@ -1102,31 +1129,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			 * is then page aligned.
 			 */
 			load_bias = ELF_PAGESTART(load_bias - vaddr);
-
-			/*
-			 * Calculate the entire size of the ELF mapping
-			 * (total_size), used for the initial mapping,
-			 * due to load_addr_set which is set to true later
-			 * once the initial mapping is performed.
-			 *
-			 * Note that this is only sensible when the LOAD
-			 * segments are contiguous (or overlapping). If
-			 * used for LOADs that are far apart, this would
-			 * cause the holes between LOADs to be mapped,
-			 * running the risk of having the mapping fail,
-			 * as it would be larger than the ELF file itself.
-			 *
-			 * As a result, only ET_DYN does this, since
-			 * some ET_EXEC (e.g. ia64) may have large virtual
-			 * memory holes between LOADs.
-			 *
-			 */
-			total_size = total_mapping_size(elf_phdata,
-							elf_ex->e_phnum);
-			if (!total_size) {
-				retval = -EINVAL;
-				goto out_free_dentry;
-			}
 		}
 
 		error = elf_load(bprm->file, load_bias + vaddr, elf_ppnt,
-- 
2.34.1

[PATCH 3/3] binfmt_elf: Honor PT_LOAD alignment for static PIE

[Kees Cook]

The p_align values in PT_LOAD were ignored for static PIE executables
(i.e. ET_DYN without PT_INTERP). This is because there is no way to
request a non-fixed mmap region with a specific alignment. ET_DYN with
PT_INTERP uses a separate base address (ELF_ET_DYN_BASE) and binfmt_elf
performs the ASLR itself, which means it can also apply alignment. For
the mmap region, the address selection happens deep within the vm_mmap()
implementation (when the requested address is 0).

The earlier attempt to implement this:

  commit 9630f0d60fec ("fs/binfmt_elf: use PT_LOAD p_align values for static PIE")
  commit 925346c129da ("fs/binfmt_elf: fix PT_LOAD p_align values for loaders")

did not take into account the different base address origins, and were
eventually reverted:

  aeb7923733d1 ("revert "fs/binfmt_elf: use PT_LOAD p_align values for static PIE"")

In order to get the correct alignment from an mmap base, binfmt_elf must
perform a 0-address load first, then tear down the mapping and perform
alignment on the resulting address. Since this is slightly more overhead,
only do this when it is needed (i.e. the alignment is not the default
ELF alignment). This does, however, have the benefit of being able to
use MAP_FIXED_NOREPLACE, to avoid potential collisions.

With this fixed, enable the static PIE self tests again.

Reported-by: H.J. Lu <hjl.tools@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215275
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Rui Salvaterra <rsalvaterra@gmail.com>
Cc: Victor Stinner <vstinner@redhat.com>
Cc: Jan Palus <jpalus@fastmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 fs/binfmt_elf.c                       | 42 +++++++++++++++++++++++----
 tools/testing/selftests/exec/Makefile |  2 +-
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 56432e019d4e..cbb07a9c02d4 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1088,10 +1088,13 @@ static int load_elf_binary(struct linux_binprm *bprm)
 				goto out_free_dentry;
 			}
 
+			/* Calculate any requested alignment. */
+			alignment = maximum_alignment(elf_phdata, elf_ex->e_phnum);
+
 			/*
 			 * There are effectively two types of ET_DYN
-			 * binaries: programs (i.e. PIE: ET_DYN with INTERP)
-			 * and loaders (ET_DYN without INTERP, since they
+			 * binaries: programs (i.e. PIE: ET_DYN with PT_INTERP)
+			 * and loaders (ET_DYN without PT_INTERP, since they
 			 * _are_ the ELF interpreter). The loaders must
 			 * be loaded away from programs since the program
 			 * may otherwise collide with the loader (especially
@@ -1111,15 +1114,44 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			 * without MAP_FIXED nor MAP_FIXED_NOREPLACE).
 			 */
 			if (interpreter) {
+				/* On ET_DYN with PT_INTERP, we do the ASLR. */
 				load_bias = ELF_ET_DYN_BASE;
 				if (current->flags & PF_RANDOMIZE)
 					load_bias += arch_mmap_rnd();
-				alignment = maximum_alignment(elf_phdata, elf_ex->e_phnum);
+				/* Adjust alignment as requested. */
 				if (alignment)
 					load_bias &= ~(alignment - 1);
 				elf_flags |= MAP_FIXED_NOREPLACE;
-			} else
-				load_bias = 0;
+			} else {
+				/*
+				 * For ET_DYN without PT_INTERP, we rely on
+				 * the architectures's (potentially ASLR) mmap
+				 * base address (via a load_bias of 0).
+				 *
+				 * When a large alignment is requested, we
+				 * must do the allocation at address "0" right
+				 * now to discover where things will load so
+				 * that we can adjust the resulting alignment.
+				 * In this case (load_bias != 0), we can use
+				 * MAP_FIXED_NOREPLACE to make sure the mapping
+				 * doesn't collide with anything.
+				 */
+				if (alignment > ELF_MIN_ALIGN) {
+					load_bias = elf_load(bprm->file, 0, elf_ppnt,
+							     elf_prot, elf_flags, total_size);
+					if (BAD_ADDR(load_bias)) {
+						retval = IS_ERR_VALUE(load_bias) ?
+							 PTR_ERR((void*)load_bias) : -EINVAL;
+						goto out_free_dentry;
+					}
+					vm_munmap(load_bias, total_size);
+					/* Adjust alignment as requested. */
+					if (alignment)
+						load_bias &= ~(alignment - 1);
+					elf_flags |= MAP_FIXED_NOREPLACE;
+				} else
+					load_bias = 0;
+			}
 
 			/*
 			 * Since load_bias is used for all subsequent loading
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index 619cff81d796..ab67d58cfab7 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -6,7 +6,7 @@ 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)
+ALIGNMENT_TESTS   := $(ALIGN_PIES) $(ALIGN_STATIC_PIES)
 
 TEST_PROGS := binfmt_script.py
 TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS)
-- 
2.34.1