Message-ID: <CAC_iWjLe7r_se30QUWE5AD3RnysYsZwrKi5oMyMdRQ4+dFehfw@mail.gmail.com>
Date: Wed, 2 Oct 2024 20:26:36 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Jeremy Linton <jeremy.linton@....com>, linux-efi@...r.kernel.org, bp@...en8.de, 
	sathyanarayanan.kuppuswamy@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] efi/libstub: measure initrd to PCR9 independent of source

Hi Ard,

On Wed, 2 Oct 2024 at 19:46, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Wed, 2 Oct 2024 at 18:36, Ilias Apalodimas
> <ilias.apalodimas@...aro.org> wrote:
> >
> > Hi Jeremy,
> >
> > On Wed, 2 Oct 2024 at 18:37, Jeremy Linton <jeremy.linton@....com> wrote:
> > >
> > > Hi,
> > >
> > > On 10/1/24 2:19 AM, Ilias Apalodimas wrote:
> > > > Thanks, Ard
> > > >
> > > > On Tue, 1 Oct 2024 at 08:59, Ard Biesheuvel <ardb@...nel.org> wrote:
> > > >>
> > > >> (cc Ilias)
> > > >>
> > > >> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@....com> wrote:
> > > >>>
> > > >>> Currently the initrd is only measured if it can be loaded using the
> > > >>> INITRD_MEDIA_GUID, if we are loading it from a path provided via the
> > > >>> command line it is never measured. Lets move the check down a couple
> > > >>> lines so the measurement happens independent of the source.
> > > >>>
> > > >>> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
> > > >>> ---
> > > >>>   drivers/firmware/efi/libstub/efi-stub-helper.c | 9 +++++----
> > > >>>   1 file changed, 5 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > >>> index de659f6a815f..555f84287f0b 100644
> > > >>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > >>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > >>> @@ -621,10 +621,6 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> > > >>>          status = efi_load_initrd_dev_path(&initrd, hard_limit);
> > > >>>          if (status == EFI_SUCCESS) {
> > > >>>                  efi_info("Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path\n");
> > > >>> -               if (initrd.size > 0 &&
> > > >>> -                   efi_measure_tagged_event(initrd.base, initrd.size,
> > > >>> -                                            EFISTUB_EVT_INITRD) == EFI_SUCCESS)
> > > >>> -                       efi_info("Measured initrd data into PCR 9\n");
> > > >>>          } else if (status == EFI_NOT_FOUND) {
> > > >>>                  status = efi_load_initrd_cmdline(image, &initrd, soft_limit,
> > > >>>                                                   hard_limit);
> > > >>> @@ -637,6 +633,11 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
> > > >>>          if (status != EFI_SUCCESS)
> > > >>>                  goto failed;
> > > >>>
> > > >>> +       if (initrd.size > 0 &&
> > > >>> +           efi_measure_tagged_event(initrd.base, initrd.size,
> > > >>> +                                    EFISTUB_EVT_INITRD) == EFI_SUCCESS)
> > > >>> +               efi_info("Measured initrd data into PCR 9\n");
> > > >
> > > > Back when we added this we intentionally left loading an initramfs
> > > > loaded via the command line out.
> > > > We wanted people to start using the LoadFile2 protocol instead of the
> > > > command line option, which suffered from various issues  -- e.g could
> > > > only be loaded if it resided in the same filesystem as the kernel and
> > > > the bootloader had to reason about the kernel memory layout.
> > > > I don't think measuring the command line option as well is going to
> > > > cause any problems, but isn't it a step backward?
> > >
> > > Thanks for looking at this. Since no one else seems to have commented, I
> > > will just express IMHO, that both methods are useful in differing
> > > circumstances.
> > >
> > > For a heavyweight Linux aware bootloader like grub/sd-boot the
> > > INITRD_MEDIA_GUID is obviously preferred. But, for booting strictly out
> > > out of a pure UEFI environment or Linux unaware bootloader (ex: UEFI
> > > shell),
> >
> > I am not sure I am following on the EfiShell. It has to run from an
> > EFI firmware somehow. The two open-source options I am aware of are
> > U-Boot and EDK2.
> > U-Boot has full support for the LoadFile2 protocol (and the
> > INITRD_MEDIA_GUID). In fact, you can add the initramfs/dtb/kernel
> > triplets as your boot options, supported by the EfiBoot manager and
> > you don't need grub/systemd boot etc.
> >
> > I don't think you can do that from EDK2 -- encode the initrd in a boot
> > option, but you can configure the initramfs to be loaded via LoadFile2
> > in OMVF via the 'initrd' shell command.
> >
> > > the commandline based initrd loader is a useful function.
> > > Because, the kernel stub should continue to serve as a complete, if
> > > minimal implementation for booting Linux out of a pure UEFI environment
> > > without additional support infrastructure (shim/grub/etc). So, it seems
> > > that unless there is a reason for divergent behavior it shouldn't exist.
> > > And at the moment, the two primary linux bootloaders grub2 and sdboot
> > > are both using the INITRD_MEDIA_GUID. Given the battering ram has been
> > > successful, it isn't a step backward.
> >
> > I am not saying we shouldn't. As I said I don't think this patch
> > breaks anything. I am just wondering if enhancing EDK2 to load the
> > initramfs via LoadFile2 for more than OVMF is a better option.
> >
>
> My original intent was to phase out initrd= entirely, because it only
> worked with the block device that the kernel image was loaded from,
> and it didn't work with mixed mode.
>
> However, both issues have been fixed:
> - you can pass initrd=<device path> and if the destination supports
> file I/O, it will be used to load the initrd (provided that your
> firmware has the TextToDevicePath protocol too, as that will be used
> to convert the provided string into something the firmware
> understands);
> - while mixed mode is a hack that should disappear over time, it now
> does support initrd= too.

Ah ok, I wasn't aware of the initrd=<device path> patches and I was
assuming you still plan to deprecate it.

>
> Even though LoadFile2 is still preferred as it is simpler and
> unambiguous, I no longer see a reason to phase out initrd=.
>
> So I think this change is reasonable.

Yes

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@...aro.org>

Powered by blists - more mailing lists