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

[Daniel Kiper]

On Wed, Jun 28, 2023 at 05:19:49PM +0200, Ard Biesheuvel wrote:
> 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?

Good point! I will add this in v2.

Daniel