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