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@arm.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");
+
 	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(initrd),
 			     (void **)&tbl);
 	if (status != EFI_SUCCESS)
-- 
2.46.1

(cc Ilias)

On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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");
> +
>         status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(initrd),
>                              (void **)&tbl);
>         if (status != EFI_SUCCESS)
> --
> 2.46.1
>

Thanks, Ard

On Tue, 1 Oct 2024 at 08:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> (cc Ilias)
>
> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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
/Ilias
> > +
> >         status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(initrd),
> >                              (void **)&tbl);
> >         if (status != EFI_SUCCESS)
> > --
> > 2.46.1
> >

Hi,

On 10/1/24 2:19 AM, Ilias Apalodimas wrote:
> Thanks, Ard
> 
> On Tue, 1 Oct 2024 at 08:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> (cc Ilias)
>>
>> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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), 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.

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

Hi Jeremy,

On Wed, 2 Oct 2024 at 18:37, Jeremy Linton <jeremy.linton@arm.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@kernel.org> wrote:
> >>
> >> (cc Ilias)
> >>
> >> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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
> >>>
>

Hi Ilias,

On 10/2/24 11:35 AM, Ilias Apalodimas wrote:
> Hi Jeremy,
> 
> On Wed, 2 Oct 2024 at 18:37, Jeremy Linton <jeremy.linton@arm.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@kernel.org> wrote:
>>>>
>>>> (cc Ilias)
>>>>
>>>> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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.

Oh, I guess the shell is a bad example because I was unaware that there 
was a initrd option in it now. I'm buying into the boot loader/boot 
manager distinction, where the manager is largely unaware of the target 
OS's specific needs (in this case, having the initrd GUID set).


> 
>> 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.

There is a separation of concerns argument here. People regularly 
complain about firmware implementations tuned for windows, but making 
the FW aware of this GUID is doing the same thing for Linux. So, IMHO 
that should be avoided, rather assuring the firmware is made as OS 
agnostic as possible, and the OS specifics are moved into the OS boot 
loader, one of which is this stub. It would make more logical sense to 
have the stub set the GUID from built in command line defaults. To be 
clear, i'm not suggesting that.

Hi Jeremy,

[...]

> >>>
> >>> 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.
>
> Oh, I guess the shell is a bad example because I was unaware that there
> was a initrd option in it now. I'm buying into the boot loader/boot
> manager distinction, where the manager is largely unaware of the target
> OS's specific needs (in this case, having the initrd GUID set).

Yes, FWIW what was added in U-Boot needs to be aware of the
Linux-specific GUID, but as far as the EFI BootOptions defined in the
Boot manager, we aren't violating anything in the EFI spec. On the
contrary, we use the _EFI_LOAD_OPTION exactly as the spec describes.

>
>
> >
> >> 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.
>
> There is a separation of concerns argument here. People regularly
> complain about firmware implementations tuned for windows, but making
> the FW aware of this GUID is doing the same thing for Linux.
> So, IMHO
> that should be avoided, rather assuring the firmware is made as OS
> agnostic as possible, and the OS specifics are moved into the OS boot
> loader, one of which is this stub. It would make more logical sense to
> have the stub set the GUID from built in command line defaults. To be
> clear, i'm not suggesting that.

I get the separation point but ....
If you do it the other way around you *force* people to use specific
OS loaders that implement OS-specific protocols. And the EFI stub is
not the problem here nor are distros that use such loaders. However,
vertical distros for embedded boards which don't need all the added
complexity have a reasonable way out.

Anyway, since Ard doesn't plan to deprecate initrd=, the patch is
reasonable, I have no objections

Thanks
/Ilias

>
>
>
>

On Wed, 2 Oct 2024 at 20:25, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jeremy,
>
> [...]
>
> > >>>
> > >>> 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.
> >
> > Oh, I guess the shell is a bad example because I was unaware that there
> > was a initrd option in it now. I'm buying into the boot loader/boot
> > manager distinction, where the manager is largely unaware of the target
> > OS's specific needs (in this case, having the initrd GUID set).
>
> Yes, FWIW what was added in U-Boot needs to be aware of the
> Linux-specific GUID, but as far as the EFI BootOptions defined in the
> Boot manager, we aren't violating anything in the EFI spec. On the
> contrary, we use the _EFI_LOAD_OPTION exactly as the spec describes.
>
> >
> >
> > >
> > >> 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.
> >
> > There is a separation of concerns argument here. People regularly
> > complain about firmware implementations tuned for windows, but making
> > the FW aware of this GUID is doing the same thing for Linux.
> > So, IMHO
> > that should be avoided, rather assuring the firmware is made as OS
> > agnostic as possible, and the OS specifics are moved into the OS boot
> > loader, one of which is this stub. It would make more logical sense to
> > have the stub set the GUID from built in command line defaults. To be
> > clear, i'm not suggesting that.
>
> I get the separation point but ....
> If you do it the other way around you *force* people to use specific
> OS loaders that implement OS-specific protocols. And the EFI stub is
> not the problem here nor are distros that use such loaders. However,
> vertical distros for embedded boards which don't need all the added
> complexity have a reasonable way out.
>
> Anyway, since Ard doesn't plan to deprecate initrd=, the patch is
> reasonable, I have no objections
>

I've queued this up now - thanks all.

On Wed, 2 Oct 2024 at 18:36, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Jeremy,
>
> On Wed, 2 Oct 2024 at 18:37, Jeremy Linton <jeremy.linton@arm.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@kernel.org> wrote:
> > >>
> > >> (cc Ilias)
> > >>
> > >> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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.

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.

Hi Ard,

On Wed, 2 Oct 2024 at 19:46, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 2 Oct 2024 at 18:36, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Jeremy,
> >
> > On Wed, 2 Oct 2024 at 18:37, Jeremy Linton <jeremy.linton@arm.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@kernel.org> wrote:
> > > >>
> > > >> (cc Ilias)
> > > >>
> > > >> On Tue, 1 Oct 2024 at 05:20, Jeremy Linton <jeremy.linton@arm.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@arm.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@linaro.org>