linux-kernel - Re: [PATCH] efi/libstub: measure initrd to PCR9 independent of source

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAC_iWjLo3j73J1x1Bw01szxN4uHUU+tPstWkYk3=+7t7DziHpw@mail.gmail.com>
Date: Wed, 2 Oct 2024 19:35:22 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Jeremy Linton <jeremy.linton@....com>
Cc: Ard Biesheuvel <ardb@...nel.org>, 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 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.

Thanks
/Ilias
>
> >
> > Thanks
> > /Ilias
> >>> +
> >>>          status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(initrd),
> >>>                               (void **)&tbl);
> >>>          if (status != EFI_SUCCESS)
> >>> --
> >>> 2.46.1
> >>>
>