Re: [PATCH v5] efi: Fix stack protector issues

[Ard Biesheuvel]

On Sat, 27 Apr 2024 at 15:08, Glenn Washburn
<development@efficientek.com> wrote:
>
> 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>

Thanks for taking care of this.

I'd ack it but that would make the signoff chain look even weirder :-)


> ---
> v5:
>  * Add missing include
>
> v4:
>  * Rebase to current master
>
> v3:
>  * Add more reasoning to comment as suggested by Vladimir
>
> Range-diff against v4:
> 1:  a79252528231 ! 1:  5731e2978906 efi: Fix stack protector issues
>     @@ grub-core/kern/efi/init.c: grub_efi_init (void)
>
>
>       ## grub-core/kern/main.c ##
>     +@@
>     +  */
>     +
>     + #include <grub/kernel.h>
>     ++#include <grub/stack_protector.h>
>     + #include <grub/misc.h>
>     + #include <grub/symbol.h>
>     + #include <grub/dl.h>
>      @@ grub-core/kern/main.c: reclaim_module_space (void)
>       void __attribute__ ((noreturn))
>       grub_main (void)
>
>  grub-core/kern/efi/init.c      | 27 ++++++++-------------------
>  grub-core/kern/main.c          | 11 +++++++++++
>  include/grub/stack_protector.h | 13 +++++++++++++
>  3 files changed, 32 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..744197785547 100644
> --- a/grub-core/kern/main.c
> +++ b/grub-core/kern/main.c
> @@ -18,6 +18,7 @@
>   */
>
>  #include <grub/kernel.h>
> +#include <grub/stack_protector.h>
>  #include <grub/misc.h>
>  #include <grub/symbol.h>
>  #include <grub/dl.h>
> @@ -265,6 +266,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 13d2657d98f5..645849e52d50 100644
> --- a/include/grub/stack_protector.h
> +++ b/include/grub/stack_protector.h
> @@ -29,6 +29,19 @@ extern void __attribute__ ((noreturn)) EXPORT_FUNC 
> (__stack_chk_fail) (void);
>  static grub_addr_t __attribute__ ((weakref("__stack_chk_guard"))) EXPORT_VAR 
> (_stack_chk_guard);
>  static void __attribute__ ((noreturn, weakref("__stack_chk_fail"))) 
> EXPORT_FUNC (_stack_chk_fail) (void);
>  #endif
> +
> +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
>