[PATCH 1/2] efi_loader: Implement FileLoad2 for initramfs loading
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Mar 12 07:58:03 CET 2020
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.
> but the linux
> module loading part, of different kernel revisions, should be there.
Can we assume that this is only the use of initrd?
> 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.
> > > +#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.
> > > @@ -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.
> > > + 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.
> > > @@ -0,0 +1,198 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2020, Linaro Limited
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <env.h>
> > > +#include <malloc.h>
> > > +#include <mapmem.h>
> > > +#include <dm.h>
> >
> > Do you need env.h and dm.h?
>
> No, I'll remove those.
>
> >
> > > +#include <fs.h>
> > > +#include <efi_loader.h>
> > > +#include <efi_load_initrd.h>
> > > +
> > > +static const efi_guid_t efi_guid_load_file2_protocol =
> > > + EFI_LOAD_FILE2_PROTOCOL_GUID;
> > > +
> > > +static efi_status_t EFIAPI
> > > +efi_load_file2_initrd(struct efi_load_file_protocol *this,
> > > + struct efi_device_path *file_path, bool boot_policy,
> > > + efi_uintn_t *buffer_size, void *buffer);
> > > +
> > > +static const struct efi_load_file_protocol efi_lf2_protocol = {
> > > + .load_file = efi_load_file2_initrd,
> > > +};
> > > +
> > > +/*
> > > + * Device path defined by Linux to identify the handle providing the
> > > + * EFI_LOAD_FILE2_PROTOCOL used for loading the initial ramdisk.
> > > + */
> > > +static const struct efi_initrd_dp dp = {
> > > + .vendor = {
> > > + {
> > > + DEVICE_PATH_TYPE_MEDIA_DEVICE,
> > > + DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
> > > + sizeof(dp.vendor),
> > > + },
> > > + EFI_INITRD_MEDIA_GUID,
> > > + },
> > > + .end = {
> > > + DEVICE_PATH_TYPE_END,
> > > + DEVICE_PATH_SUB_TYPE_END,
> > > + sizeof(dp.end),
> > > + }
> > > +};
> > > +
> > > +/**
> > > + * get_file_size() - retrieve the size of initramfs, set efi status on error
> > > + *
> > > + * @dev: device to read from. i.e "mmc"
> > > + * @part: device partition. i.e "0:1"
> > > + * @file: name fo file
> > > + * @status: EFI exit code in case of failure
> > > + *
> > > + * Return: size of file
> > > + */
> > > +static loff_t get_file_size(const char *dev, const char *part, const char *file,
> > > + efi_status_t *status)
> > > +{
> > > + loff_t sz = 0;
> > > + int ret;
> > > +
> > > + ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> > > + if (ret) {
> > > + *status = EFI_NO_MEDIA;
> > > + goto out;
> > > + }
> > > +
> > > + ret = fs_size(file, &sz);
> > > + if (ret) {
> > > + sz = 0;
> > > + *status = EFI_NOT_FOUND;
> > > + goto out;
> > > + }
> > > +
> > > +out:
> > > + return sz;
> > > +}
> > > +
> > > +/**
> > > + * load_file2() - get information about random number generation
> >
> > Please correct the message.
> >
>
> Ooops
>
> > > + *
> > > + * This function implement the LoadFile2() service in order to load an initram
> > > + * disk requested by the Linux kernel stub.
> > > + * See the UEFI spec for details.
> > > + *
> > > + * @this: loadfile2 protocol instance
> > > + * @file_path: relative path of the file. "" in this case
> > > + * @boot_policy: must be false for Loadfile2
> > > + * @buffer_size: size of allocated buffer
> > > + * @buffer: buffer to load the file
> > > + *
> > > + * Return: status code
> > > + */
> > > +static efi_status_t EFIAPI
> > > +efi_load_file2_initrd(struct efi_load_file_protocol *this,
> > > + struct efi_device_path *file_path, bool boot_policy,
> > > + efi_uintn_t *buffer_size, void *buffer)
> > > +{
> > > + const char *filespec = CONFIG_EFI_INITRD_FILESPEC;
> > > + efi_status_t status = EFI_NOT_FOUND;
> > > + loff_t file_sz = 0, read_sz = 0;
> > > + char *dev, *part, *file;
> > > + char *s;
> > > + int ret;
> > > +
> > > + EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
> > > + buffer_size, buffer);
> > > +
> > > + s = strdup(filespec);
> > > + if (!s)
> > > + goto out;
> >
> > 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.
Thanks,
-Takahiro Akashi
> > > +
> > > + if (!this || this != &efi_lf2_protocol ||
> >
> > Please use guidcmp() here.
> >
>
> Sure
>
> > > + !buffer_size) {
> > > + if (!file_sz)
> [...]
> > > + goto out;
> >
> > ditto.
> >
>
> Sure
>
> > > +
> > > + if (!buffer || *buffer_size < file_sz) {
> [...]
> > > if (ret != EFI_SUCCESS)
> > > --
> > > 2.25.1
> > >
>
>
> Regards
> /Ilias
More information about the U-Boot
mailing list