On Sat, 27 Apr 2024 at 15:08, Glenn Washburn <developm...@efficientek.com> wrote: > > From: Ard Biesheuvel <a...@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 <a...@kernel.org> > Reviewed-by: Glenn Washburn <developm...@efficientek.com> > Signed-off-by: Glenn Washburn <developm...@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 > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel