Re: [RFC PATCH] efi: Disable stack smashing protection on grub_efi_init()

[Glenn Washburn]

On Tue, 18 Jul 2023 00:47:14 -0500
Glenn Washburn <development@efficientek.com> wrote:

> GCC is electing to instrument grub_efi_init() to give it stack smashing
> protection when configuring with --enable-stack-protector on the x86_64-efi
> target. In the function prologue, the canary at the top of the stack frame
> is set to the value of the stack guard. And in the epilogue, the canary is
> checked to verify if it is equal to the guard and if not to call the stack
> check fail function. The issue is that grub_efi_init() sets up the guard
> by initializing it with random bytes, if the firmware supports the RNG
> protocol. So in its prologue the canary will be set with the value of the
> uninitialized guard, likely NULL bytes. Then the guard is initialized, and
> finally the epilogue checks the canary against the guard, which will almost
> certainly be different. This causes the code path for a smashed stack to be
> taken, causing the machine to print out a message that stack smashing was
> detected, wait 5 seconds, and then reboot. Disable grub_efi_init()
> instrumentation so there is no stack smashing false positive generated.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> I recently discovered that my OVMF (debian version 2020.11-2+deb11u1) was
> not initializing the stack guard because the firmware was did enable support
> for the RNG protocol. It turns out that the RNG protocol is not enabled
> unless a source of randomness is found, which can be added with the QEMU
> arguments "-device virtio-rng-pci". This issue did not occur when the
> guard was not initialized in grub_efi_init() because the uninitialized
> value of the guard would be used for the canary, and sinice nothing changed,
> the canary and guard would be the same at the end of grub_efi_init().
> 
> Initially to solve this, I tried to set the canary in grub_efi_init() after
> the guard gets initialized. The problem I ran into is that gcc outputs
> instructions that use the base pointer register as a general purpose
> register. I couldn't come up with another method of generically figuring
> out where the start of the frame was so that I could set the canary. The
> rsp register is offset by the functions local variables which are variable
> as code is inlined or otherwise changes. Knowing the offset of the stack
> pointer to the frame start, I could properly set the canary. But again,

I was limiting my thinking to having the frame detection be solely
within grub_efi_init(). I can think of a couple other options, but they
are kludges, and may not be as applicable to non-x86.

 1. From the caller of grub_efi_init(), pass in the value of %esp as an
    argument via an asm statement. This requires changing the
    declaration to "void grub_efi_init (grub_addr_t *frame_base). Then
    add frame_base-2 should be the canary.
    * I mainly don't like this because changing the declaration is ugly.

 2. Similar to (1), but store in unused register and then use asm in
    grub_efi_init() to get at the value.
    * This seems hacky, since this is effectively passing an extra
       argument that is invisible C function declaration.

 3. Similar to (1), but in caller store %esp to a global variable and
    then read global from grub_efi_init().
    * Less hacky, but still kinda hacky and requires an unnecessary
      memory write.

 4. Statically initialize guard to unique value. Then in
    stack_protector_init() search stack memory for value with
    statically initialized guard value and assume if found that its a
    canary. Once found the canary, overwrite it with the new random
    guard value. The stack is short at this point, so we can limit the
    search to say 8 pointer sizes.
    * I have changes that implementing this and it works. This is fairly
      clean, in that no asm calls are needed, but its still kinda hacky
      in that it only checks up to a certain depth, which might not be
      enough if the code (radically) changes. My working implementation
      assumes that the canary has pointer size alignment, which may not
      be true on some platforms(?).

Of these, I like (4) the best, as it involves no inline assembly and
feels minimally intrusive.

Glenn

> if local variables are added or subtracted, including in inlined functions
> called, that offset will need to be regenerated. And its fragile because
> different verions of GCC or optimization levels may change this offset also.
> 
> If there was a robust way to set the canary, I think that would be preferable
> so that we can get stack smashing support on the function. Any one have ideas?
> 
> Another thing that has troubled me is why has no one else seen this behavior
> in the last several years that this code has been active? Do distros already
> have a patch for this? Is the RNG protocol supported by virtual no bare
> metal firmwares? I don't hit this issue on test machine, so I'm lead to
> believe that it must not support the RNG protocol (but I've yet to check).
> Are there other explanations for this that I'm missing?
> 
> Glenn
> ---
>  grub-core/kern/efi/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index 6fe1ff8c868e..e759cc315b85 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -102,7 +102,7 @@ stack_protector_init (void)
>  
>  grub_addr_t grub_modbase;
>  
> -void
> +__attribute__ ((__optimize__ ("-fno-stack-protector"))) void
>  grub_efi_init (void)
>  {
>    grub_modbase = grub_efi_section_addr ("mods");