[PATCH v3] efi: Fix stack protector issues

From: Glenn Washburn <development@efficientek.com>
To: grub-devel@gnu.org, Daniel Kiper <dkiper@net-space.pl>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>,
	Glenn Washburn <development@efficientek.com>
Subject: [PATCH v3] efi: Fix stack protector issues
Date: Sat, 27 Apr 2024 05:18:35 -0500	[thread overview]
Message-ID: <20240427101835.532252-1-development@efficientek.com> (raw)

From: Ard Biesheuvel <ardb@kernel.org>

The 'ground truth' stack protector cookie value is kept in a global
variable, and loaded in every function prologue and epilogue to store
it into resp. compare it with the stack slot holding the cookie.

If the comparison fails, the program aborts, and this might occur
spuriously when the global variable changes values between the entry and
exit of a function. This implies that assigning the global variable at
boot should not involve any instrumented function calls, unless special
care is taken to ensure that the live call stack is synchronized, which
is non-trivial.

So avoid any function calls, including grub_memcpy(), which is
unnecessary given that the stack cookie is always a suitably aligned
variable of the native word size.

While at it, leave the last byte 0x0 to avoid inadvertent unbounded
strings on the stack.

Note that the use of __attribute__((optimize)) is described as
unsuitable for production use in the GCC documentation, so let's drop
this as well now that it is no longer needed.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Glenn Washburn <development@efficientek.com>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
Since Ard hasn't been heard from in over 3 months. I've taken the liberty
to add more reasoning to add comment as requested by Vladimir. I hope this
will get the ball rolling on getting this patch included.

Glenn

Range-diff against v2:
1:  adfa6d610b34 ! 1:  9dbc35848323 efi: Fix stack protector issues
    @@ Commit message
         unsuitable for production use in the GCC documentation, so let's drop
         this as well now that it is no longer needed.
     
    -    Cc: Glenn Washburn <development@efficientek.com>
    -    Cc: Daniel Kiper <dkiper@net-space.pl>
         Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
    +    Reviewed-by: Glenn Washburn <development@efficientek.com>
     
      ## grub-core/kern/efi/init.c ##
     @@ grub-core/kern/efi/init.c: static grub_efi_char16_t stack_chk_fail_msg[] =
    @@ grub-core/kern/main.c: reclaim_module_space (void)
      grub_main (void)
      {
     +#ifdef GRUB_STACK_PROTECTOR
    -+  /* This call can only be made from a function that does not return. */
    ++  /*
    ++   * This call should only be made from a function that does not return because
    ++   * functions that return will get instrumented to check that the stack cookie
    ++   * does not change and this call will change the stack cookie. Thus a stack
    ++   * guard failure will be triggered.
    ++   */
     +  grub_update_stack_guard ();
     +#endif
     +

 grub-core/kern/efi/init.c      | 27 ++++++++-------------------
 grub-core/kern/main.c          | 10 ++++++++++
 include/grub/stack_protector.h | 13 +++++++++++++
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 6c54af6e79e5..1637077e1e96 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -39,12 +39,6 @@ static grub_efi_char16_t stack_chk_fail_msg[] =
 
 static grub_guid_t rng_protocol_guid = GRUB_EFI_RNG_PROTOCOL_GUID;
 
-/*
- * Don't put this on grub_efi_init()'s local stack to avoid it
- * getting a stack check.
- */
-static grub_efi_uint8_t stack_chk_guard_buf[32];
-
 /* Initialize canary in case there is no RNG protocol. */
 grub_addr_t __stack_chk_guard = (grub_addr_t) GRUB_STACK_PROTECTOR_INIT;
 
@@ -77,8 +71,8 @@ __stack_chk_fail (void)
   while (1);
 }
 
-static void
-stack_protector_init (void)
+grub_addr_t
+grub_stack_protector_init (void)
 {
   grub_efi_rng_protocol_t *rng;
 
@@ -87,23 +81,20 @@ stack_protector_init (void)
   if (rng != NULL)
     {
       grub_efi_status_t status;
+      grub_addr_t guard = 0;
 
-      status = rng->get_rng (rng, NULL, sizeof (stack_chk_guard_buf),
-			     stack_chk_guard_buf);
+      status = rng->get_rng (rng, NULL, sizeof (guard) - 1,
+		             (grub_efi_uint8_t *) &guard);
       if (status == GRUB_EFI_SUCCESS)
-	grub_memcpy (&__stack_chk_guard, stack_chk_guard_buf, sizeof (__stack_chk_guard));
+	return guard;
     }
-}
-#else
-static void
-stack_protector_init (void)
-{
+  return 0;
 }
 #endif
 
 grub_addr_t grub_modbase;
 
-__attribute__ ((__optimize__ ("-fno-stack-protector"))) void
+void
 grub_efi_init (void)
 {
   grub_modbase = grub_efi_section_addr ("mods");
@@ -111,8 +102,6 @@ grub_efi_init (void)
      messages.  */
   grub_console_init ();
 
-  stack_protector_init ();
-
   /* Initialize the memory management system.  */
   grub_efi_mm_init ();
 
diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
index 731c07c2901a..5478316400ce 100644
--- a/grub-core/kern/main.c
+++ b/grub-core/kern/main.c
@@ -265,6 +265,16 @@ reclaim_module_space (void)
 void __attribute__ ((noreturn))
 grub_main (void)
 {
+#ifdef GRUB_STACK_PROTECTOR
+  /*
+   * This call should only be made from a function that does not return because
+   * functions that return will get instrumented to check that the stack cookie
+   * does not change and this call will change the stack cookie. Thus a stack
+   * guard failure will be triggered.
+   */
+  grub_update_stack_guard ();
+#endif
+
   /* First of all, initialize the machine.  */
   grub_machine_init ();
 
diff --git a/include/grub/stack_protector.h b/include/grub/stack_protector.h
index c88dc00b5f97..9212bb4a6f9a 100644
--- a/include/grub/stack_protector.h
+++ b/include/grub/stack_protector.h
@@ -25,6 +25,19 @@
 #ifdef GRUB_STACK_PROTECTOR
 extern grub_addr_t EXPORT_VAR (__stack_chk_guard);
 extern void __attribute__ ((noreturn)) EXPORT_FUNC (__stack_chk_fail) (void);
+
+grub_addr_t
+grub_stack_protector_init (void);
+
+static inline __attribute__((__always_inline__))
+void grub_update_stack_guard (void)
+{
+  grub_addr_t guard;
+
+  guard = grub_stack_protector_init ();
+  if (guard)
+     __stack_chk_guard = guard;
+}
 #endif
 
 #endif /* GRUB_STACK_PROTECTOR_H */
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
