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