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

[Glenn Washburn]

On Fri, 12 Jan 2024 18:42:19 +0100
Ard Biesheuvel <ardb+git@google.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.

What is "resp."? It seems like an editing mistake here. Regardless I'm
not understanding this sentence even though I think I know what you
want to say.

> 
> 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.

I like this.

> 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.

While this is true, I don't think this its the main point of the 0x0
byte. My understanding is what it does is prevent bypassing stack
protection via canary forgery from buffer overflows in functions with a
NULL check. The would be forger must write a NULL byte, which
will terminate the buffer writing, thus not corrupting the return
pointer. Not writing the NULL byte corrupts the canary, thus triggering
the stack protection mechanism. This though makes some assumptions
about a well implemented stack protection mechanism because the buffer
overflow could blow past the whole stack and write into other areas of
memory which might break even the stack protection mechanism. I would
like to pretend that the implementers are aware of this and avoid this
possibility, but perhaps this is more of a risk for projects like GRUB
that are using a stack protection mechanism in an environment that (I
presume) it was not specifically designed for. I guess we'll cross our
fingers until someone looks at this more closely (maybe someone already
has and I'm not aware of it).

Glenn

> 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.
> 
> Cc: Glenn Washburn <development@efficientek.com>
> Cc: Daniel Kiper <dkiper@net-space.pl>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> v2:
> - init stack guard before machine init
> - update commit log to explain that it is possible in theory to fix up a
>   live call stack
> 
>  grub-core/kern/efi/init.c      | 27 ++++++--------------
>  grub-core/kern/main.c          |  5 ++++
>  include/grub/stack_protector.h | 13 ++++++++++
>  3 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 6c54af6e7..1637077e1 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 731c07c29..5db504e6e 100644
> --- a/grub-core/kern/main.c
> +++ b/grub-core/kern/main.c
> @@ -265,6 +265,11 @@ reclaim_module_space (void)
>  void __attribute__ ((noreturn))
>  grub_main (void)
>  {
> +#ifdef GRUB_STACK_PROTECTOR
> +  /* This call can only be made from a function that does not return. */
> +  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 c88dc00b5..9212bb4a6 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 */