[PATCH 0/1] MR3708: loader: Don't return from __stack_chk_fail() stub (which GCC expects to be noreturn) in preloader.

[Jinoh Kang (＠iamahuman)]

GCC expects __stack_chk_fail() to be noreturn[^1].  Returning from
__stack_chk_fail() can thus lead to subtle failures.

When crashing, use a volatile NULL pointer dereference.  Ideally we
would like to "abort()" here, but doing so would require two more
syscall definitions just for a cold function.  After all, the function
isn't even used at all if -fno-stack-protector is specified.

Also, don't say "stack smashing" (unlike glibc).  The preloader
currently initializes the stack canary value to a fixed value (0), which
serves little value in protecting against actual buffer overrun attacks.

[^1]: https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gccint/Stack-Smashing-Protection.ht...

[PATCH 1/1] loader: Don't return from __stack_chk_fail() stub in preloader.

[Jinoh Kang]

From: Jinoh Kang

GCC expects __stack_chk_fail() to be noreturn[1].  Returning from
__stack_chk_fail() can thus lead to subtle failures.

When crashing, use a volatile NULL pointer dereference.  Ideally we
would like to "abort()" here, but doing so would require two more
syscall definitions just for a cold function.  After all, the function
isn't even used at all if -fno-stack-protector is specified.

Also, don't say "stack smashing" (unlike glibc).  The preloader
currently initializes the stack canary value to a fixed value (0), which
serves little value in protecting against actual buffer overrun attacks.

[1]: https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gccint/Stack-Smashing-Protection.ht...
---
 loader/preloader.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/loader/preloader.c b/loader/preloader.c
index 635e85ee7cb..64db9205ac0 100644
--- a/loader/preloader.c
+++ b/loader/preloader.c
@@ -175,11 +175,6 @@ struct wld_auxv
  */
 void __bb_init_func(void) { return; }

-/* similar to the above but for -fstack-protector */
-void *__stack_chk_guard = 0;
-void __stack_chk_fail_local(void) { return; }
-void __stack_chk_fail(void) { return; }
-
 #ifdef __i386__

/* data for setting up the glibc-style thread-local storage in %gs */
@@ -759,6 +754,34 @@ static __attribute__((noreturn,format(printf,1,2))) void fatal_error(const char
     wld_exit(1);
 }

+/*
+ * The __stack_chk_fail is a no-return function only called when file is
+ * compiled with gcc flags "-fstack-protector".  This function is normally
+ * provided by libc's startup files, but since we build the preloader with
+ * "-nostartfiles -nodefaultlibs", we have to provide our own version,
+ * otherwise linker fails.
+ */
+unsigned long __stack_chk_guard = 0;
+
+void __attribute__((noreturn)) __stack_chk_fail(void)
+{
+    const char message[] = "preloader: stack overrun detected, crashing\n";
+
+    /* Avoid using non-syscall functions that can re-enter this function */
+    wld_write(2, message, sizeof(message) - 1);
+
+    /* Deliberate induce crash and possibly dump core */
+    *(volatile char *)0;
+
+    /* Last resort if the zero page turns out to be actually readable */
+    wld_exit(1);
+}
+
+void __attribute__((noreturn)) __stack_chk_fail_local(void)
+{
+    __stack_chk_fail();
+}
+
 #ifdef DUMP_AUX_INFO
 /*
  *  Dump interesting bits of the ELF auxv_t structure that is passed