Re: [PATCH] xen/efi: Do not check kernel signature if it was embedded

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 19, 2025 at 01:33:20PM +0100, Frediano Ziglio wrote:
> On Thu, Jun 19, 2025 at 1:17 PM Marek Marczykowski-Górecki
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Jun 18, 2025 at 07:46:28PM +0100, Frediano Ziglio wrote:
> > > Using UKI it's possible to embed Linux kernel into xen.efi file.
> > > In this case the signature for Secure Boot is applied to the
> > > whole xen.efi, including the kernel.
> > > So checking for specific signature for the kernel is not
> > > needed.
> > > In case Secure Boot is not enabled there's no reason to check
> > > kernel signature.
> >
> > The last sentence (here and in the comment below) seem to be unrelated
> > to this change - it's more about shim lock protocol being available,
> > which this patch doesn't change.
> >
> 
> Should I just remove the sentence?

Yes, and reword the code comment a bit.

> Beside that sentence, any issue with this change?

Other than that it looks fine for me.

> > > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > > ---
> > >  xen/common/efi/boot.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > > index e39fbc3529..7077af3f5d 100644
> > > --- a/xen/common/efi/boot.c
> > > +++ b/xen/common/efi/boot.c
> > > @@ -1291,6 +1291,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> > > ImageHandle,
> > >      bool base_video = false;
> > >      const char *option_str;
> > >      bool use_cfg_file;
> > > +    bool kernel_was_verified = false;
> > >      int dt_modules_found;
> > >
> > >      __set_bit(EFI_BOOT, &efi_flags);
> > > @@ -1461,6 +1462,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> > > ImageHandle,
> > >              read_file(dir_handle, s2w(&name), &kernel, option_str);
> > >              efi_bs->FreePool(name.w);
> > >          }
> > > +        else
> > > +        {
> > > +            /*
> > > +             * As kernel was embedded it was either verified for Secure 
> > > Boot
> > > +             * or Secure Boot is not enabled.
> > > +             */
> > > +            kernel_was_verified = true;
> > > +        }
> > >
> > >          if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> > >          {
> > > @@ -1534,6 +1543,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> > > ImageHandle,
> > >       * verify it.
> > >       */
> > >      if ( kernel.ptr &&
> > > +         !kernel_was_verified &&
> > >           !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> > >                                             (void **)&shim_lock)) &&
> > >           (status = shim_lock->Verify(kernel.ptr, kernel.size)) != 
> > > EFI_SUCCESS )
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Best Regards,
> > Marek Marczykowski-Górecki
> > Invisible Things Lab

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature