[PATCH 1/2] efi_loader: Implement FileLoad2 for initramfs loading

[Ilias Apalodimas]

On Thu, Mar 12, 2020 at 03:58:03PM +0900, AKASHI Takahiro wrote:
> On Thu, Mar 12, 2020 at 08:29:38AM +0200, Ilias Apalodimas wrote:
> > Akashi-san,
> > 
> > On Thu, Mar 12, 2020 at 02:28:45PM +0900, AKASHI Takahiro wrote:
> > > Ilias,
> > > 
> > > I haven't followed this thread.
> > > 
> > > On Fri, Feb 21, 2020 at 09:55:45AM +0200, Ilias Apalodimas wrote:
> > > > Following kernel's proposal for an arch-agnostic initrd loading
> > > > mechanism [1] let's implement the U-boot counterpart.
> > > > This new approach has a number of advantages compared to what we did up
> > > > to now. The file is loaded into memory only when requested limiting the
> > > > area of TOCTOU attacks. Users will be allowed to place the initramfs
> > > > file on any u-boot accessible partition instead of just the ESP one.
> > > > Finally this is an attempt of a generic interface across architectures
> > > > in the linux kernel so it makes sense to support that.
> > > > 
> > > > The file location is intentionally only supported as a config option
> > > > argument(CONFIG_EFI_INITRD_FILESPEC), in an effort to enhance security.
> > > > Although U-boot is not responsible for verifying the integrity of the
> > > > initramfs, we can enhance the offered security by only accepting a
> > > > built-in option, which will be naturally verified by UEFI Secure Boot.
> > > > This can easily change in the future if needed and configure that via ENV
> > > > or UEFI variable.
> > > 
> > > What if we have several bootable images with different initrd
> > > required at the same time? For example, we may install ubuntu 16.04,
> > > 18.04 etc to the same device (and then boot one of them via grub).
> > > 
> > > Since the guid is unique, U-Boot doesn't have any measure to identify which
> > > initrd be exported via FILE2_PROTOCOL.
> > > In this sense, I don't think using a config option is a good idea in general.
> > 
> > Can't you concatenate multiple cpio archives for this? I haven't checked this
> > personally but i remember distros doing it to add firmware files they need for
> > example. It might not be ideal because you may need different user-space
> > program versions or different versions of the same script(??)
> 
> I wonder think why it is not a problem.
> 

I never said it wasn't, on the contrary, i pointed out cases were
concatenating cpio's might not be enough.

> > but the linux 
> > module loading part, of different kernel revisions, should be there.
> 
> Can we assume that this is only the use of initrd?
> 

Of course not.

> > I don't have any strong preference on this anyway hence my commit
> > message, it's just another naive protection, since the user can always replace
> > the initramfs using the same name and break out of this. I never needed multiple
> > initrd files so I preferred the config option. If people need a config option
> > it's easily extensible.
> 
> How? I don't think it's trivial.

Parse an env variable and override the config option?
Something like:
setenv efi_initrd 'mmc 0:1 foo.cpio.gz' -> env_get("efi_initrd")
wouldn't work?

> 
> > > > +#define EFI_LOAD_FILE_PROTOCOL_GUID \
> > [...]
> > > > +	EFI_GUID(0x56ec3091, 0x954c, 0x11d2, \
> > > > +		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> > > 
> > > Do you need this definition?
> > > 
> > 
> > Not really it just felt weird adding LOAD_FILE2 without the LOAD_FILE
> > definition.
> > 
> > > > +#define EFI_LOAD_FILE2_PROTOCOL_GUID \
> > > > +	EFI_GUID(0x4006c0c1, 0xfcb3, 0x403e, \
> > > > +		 0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d)
> > > > +
> > > > --- /dev/null
> > [...]
> > > > +++ b/include/efi_load_initrd.h
> > > 
> > > It might be unlikely, but the file name should be more generic
> > > for potential enhancement.
> > > 
> > 
> > This is defining a very specific GUID for initramfs that Linux consumes to load
> > the file, I'd rather keep it as is.
> 
> I didn't intent the guid, but a file name.

Yes my response is for the file name. It serves a sole purpose, define the 
explicit GUID linux expects.

> > > > @@ -0,0 +1,25 @@
> > [...]
> > > > +
> > > > +config EFI_INITRD_FILESPEC
> > > > +	string "initramfs path"
> > > > +	default "host 0:1 initrd"
> > > 
> > > "host" interface makes sense only on sandbox, which will not be expected
> > > to be able to boot any linux kernel.
> > > So this default value is inappropriate anyway.
> > > 
> > 
> > Not really. The user *must* have an idea on what he is doing and this is the
> > default config we need to run on the sandbox. So it serves both as an example
> > and a defult config for the sandbox testing.
> 
> I don't think we can boot a linux kernel from U-Boot on sandbox.
> 

We run the sefltest though, which is replicating the Linux efi stub 
functionality.

> > > > +	depends on EFI_LOAD_FILE2_INITRD
> > > > +	help
> > > > +	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
> > > > +
> > > >  endif
> > > > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > > > index 04dc8648512b..9b3b70447336 100644
> > > > --- a/lib/efi_loader/Makefile
> > > > +++ b/lib/efi_loader/Makefile
> > > > @@ -43,3 +43,4 @@ obj-$(CONFIG_NET) += efi_net.o
> > > >  obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> > > >  obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> > > >  obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> > > > +obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
> > > > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> > > > new file mode 100644
> > > > index 000000000000..574a83d7e308
> > > > --- /dev/null
> > > > +++ b/lib/efi_loader/efi_load_initrd.c
> > > 
> > > I'd prefer efi_file_initrd.c for similarity to efi_file.c, or
> > > more specifically efi_linux.c.
> > > 
> > 
> > I can send a patch renaming those but i think it's pointless.
> > The filename describes exactly what's the intent imho.
> 
> What if we will want to add another FILE2_PROTOCOL or linux-specifc
> hack?
> Now I believe that OS (or platform)-dependent implementation
> should go into a separate directory, like lib/efi_linux/...
> for clarification.
> 

I am not against that, i just think the tons of comments are already enough. 
But i see the point in the long run, assuming we'll add enough OS-specific
options to justify this.

> > > > @@ -0,0 +1,198 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*

[...]

> > > I think that we should return a better error code rather than NOT_FOUND.
> > > 
> > 
> > Like? The spec has a specific set of returns codes you can use there, 
> > EFI_UNSUPPORTED
> > EFI_INVALID_PARAMETER
> > EFI_NO_MEDIA
> > EFI_DEVICE_ERROR
> > EFI_NO_RESPONSE
> > EFI_NOT_FOUND
> > EFI_ABORTED
> > EFI_BUFFER_TOO_SMALL
> > 
> > Is there anything you like better?
> 
> In most of all cases, OUT_OF_RESOURCES.

If i am not reading the spec wrong you *can't* return that here and even if you
could, the majority of error cases fail if the file was not found.
Maybe apart from this:
ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
if (ret) {
	status = EFI_NO_MEDIA;
   	goto out;
}


Thanks
/Ilias