Re: [PATCH 2/2] efi: Fallback to legacy mode if shim is loaded on x86 archs

[Ard Biesheuvel]

On Wed, 28 Jun 2023 at 15:29, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> The LoadImage() provided by the shim does not consult MOK when loading
> an image. So, simply signature verification fails when it should not.
> This means we cannot use Linux EFI stub to start the kernel when the
> shim is loaded. We have to fallback to legacy mode on x86 architectures.
> This is not possible on other architectures due to lack of legacy mode.
>
> This is workaround which should disappear when the shim provides
> LoadImage() which looks up MOK during signature verification.
>
> On the occasion align constants in include/grub/efi/sb.h.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/kern/efi/sb.c      | 10 ++++++++++
>  grub-core/loader/efi/linux.c | 13 +++++++++++++
>  include/grub/efi/sb.h        |  5 ++++-
>  3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c
> index 80cfa0888..60550a6da 100644
> --- a/grub-core/kern/efi/sb.c
> +++ b/grub-core/kern/efi/sb.c
> @@ -32,6 +32,8 @@
>
>  static grub_guid_t shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
>
> +static bool shim_lock_enabled = false;
> +
>  /*
>   * Determine whether we're in secure boot mode.
>   *
> @@ -215,6 +217,14 @@ grub_shim_lock_verifier_setup (void)
>    /* Enforce shim_lock_verifier. */
>    grub_verifier_register (&shim_lock_verifier);
>
> +  shim_lock_enabled = true;
> +
>    grub_env_set ("shim_lock", "y");
>    grub_env_export ("shim_lock");
>  }
> +
> +bool
> +grub_is_shim_lock_enabled (void)
> +{
> +  return shim_lock_enabled;
> +}
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index c1eef7c98..5fb2ad01f 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -29,6 +29,7 @@
>  #include <grub/efi/fdtload.h>
>  #include <grub/efi/memory.h>
>  #include <grub/efi/pe32.h>
> +#include <grub/efi/sb.h>
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
>  #include <grub/verify.h>
> @@ -458,6 +459,18 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>
>    grub_dl_ref (my_mod);
>
> +#if defined(__i386__) || defined(__x86_64__)
> +  if (grub_is_shim_lock_enabled () == true)
> +    {
> +      err = grub_cmd_linux_x86_legacy (cmd, argc, argv);
> +

Even if we only have a fallback on x86, we may encounter this
condition on other architectures too, so I think the check should be
generic, and only the fallback specific.

Not sure what to do on other architecures, though - there is no
backward compatibility concern here (at least not wrt users of
mainline GRUB), so we could still *try* to use the firmware's
loadimage/startimage, but perhaps issue a diagnostic message at the
very least?


> +      if (err == GRUB_ERR_NONE)
> +       return GRUB_ERR_NONE;
> +      else
> +       goto fail;
> +    }
> +#endif
> +
>    if (argc == 0)
>      {
>        grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> diff --git a/include/grub/efi/sb.h b/include/grub/efi/sb.h
> index 30c4335bb..49a9ad01c 100644
> --- a/include/grub/efi/sb.h
> +++ b/include/grub/efi/sb.h
> @@ -22,7 +22,7 @@
>  #include <grub/types.h>
>  #include <grub/dl.h>
>
> -#define GRUB_EFI_SECUREBOOT_MODE_UNSET 0
> +#define GRUB_EFI_SECUREBOOT_MODE_UNSET         0
>  #define GRUB_EFI_SECUREBOOT_MODE_UNKNOWN       1
>  #define GRUB_EFI_SECUREBOOT_MODE_DISABLED      2
>  #define GRUB_EFI_SECUREBOOT_MODE_ENABLED       3
> @@ -31,6 +31,9 @@
>  extern grub_uint8_t
>  EXPORT_FUNC (grub_efi_get_secureboot) (void);
>
> +extern bool
> +EXPORT_FUNC (grub_is_shim_lock_enabled) (void);
> +
>  extern void
>  grub_shim_lock_verifier_setup (void);
>  #else
> --
> 2.11.0
>