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