Re: [PATCH] efi: Fix stack protector issues

[Ard Biesheuvel]

Hi Glenn,

On Thu, 28 Dec 2023 at 03:26, Glenn Washburn
<development@efficientek.com> wrote:
>
> On Sat, 23 Dec 2023 12:45:35 +0100
> Ard Biesheuvel via Grub-devel <grub-devel@gnu.org> 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 must not involve any instrumented function calls.
>
> Not quite true, I had an alternative patch that searched the stack for
> the old canary and replaced it with the new canary. This implementation
> is better, though less general (which doesn't add any value).
>

I don't think exposing an API that circumvents the C runtime and
'fixes' a call stack by replacing invalid cookie values with valid
ones belongs in production code. It is not trivial to implement
generically, and has the potential to do more harm than good if an
attacker manages to access it.

...

> > 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.
>
> Good to know. I didn't particularly like that change either.
>
> The added benefit of this change is that it looks like it will make it
> easier to add stack protection to other targets.
>

Indeed.

> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  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..1244fa84f 100644
> > --- a/grub-core/kern/main.c
> > +++ b/grub-core/kern/main.c
> > @@ -270,6 +270,11 @@ grub_main (void)
> >
> >    grub_boot_time ("After machine init.");
> >
> > +#ifdef GRUB_STACK_PROTECTOR
> > +  /* This call can only be made from a function that does not return. */
> > +  grub_update_stack_guard ();
> > +#endif
> > +
>
> Why don't we do this before grub_machine_init () so that
> grub_efi_init() is covered?

Because we use locate_protocol(), which I don't think we should be
using before grub_efi_init(), even if we could theoretically (but I
haven't checked whether this is the case)

The important thing is to initialize this before accepting any user
input, so I don't think grub_machine_init() is something that really
needs the stack protector to begin with.

>  grub_update_stack_guard() and
> __stack_chk_fail() do not appear to rely anything done in
> grub_efi_init().
>

IMO after grub_machine_init is early enough, and this makes it more
likely that the stack protector support can be ported to other
platforms too.


> >    /* This breaks flicker-free boot on EFI systems, so disable it there. */
> >  #ifndef GRUB_MACHINE_EFI
> >    /* Hello.  */
> > 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 */