From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mqH2w-0000ag-EA for mharc-grub-devel@gnu.org; Thu, 25 Nov 2021 10:52:26 -0500 Received: from eggs.gnu.org ([209.51.188.92]:50058) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mqH2v-0000YE-0k for grub-devel@gnu.org; Thu, 25 Nov 2021 10:52:25 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:45273) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1mqH2s-0006Kp-AJ for grub-devel@gnu.org; Thu, 25 Nov 2021 10:52:24 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:45636 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2098892AbhKYPwM (ORCPT ); Thu, 25 Nov 2021 16:52:12 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 25 Nov 2021 16:52:09 +0100 From: Daniel Kiper To: Nikita Ermakov Cc: GRUB development mailing list , Ard Biesheuvel , Heinrich Schuchardt , Leif Lindholm , Fu Wei Subject: Re: [PATCH v3 3/7] efi: implemented LoadFile2 initrd loading protocol for Linux Message-ID: <20211125155209.t3kxzu7japkttsqq@tomti.i.net-space.pl> References: <20211028203118.7908-1-arei@altlinux.org> <20211028203118.7908-4-arei@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211028203118.7908-4-arei@altlinux.org> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Nov 2021 15:52:25 -0000 On Thu, Oct 28, 2021 at 11:31:16PM +0300, Nikita Ermakov wrote: > From: Ard Biesheuvel > > Recent Linux kernels will invoke the LoadFile2 protocol installed on > a well-known vendor media path to load the initrd if it is exposed by > the firmware. Using this method is preferred for two reasons: > - the Linux kernel is in charge of allocating the memory, and so it can > implement any placement policy it wants (given that these tend to > change between kernel versions), > - it is no longer necessary to modify the device tree provided by the > firmware. > > So let's install this protocol when handling the 'initrd' command if > such a recent kernel was detected (based on the PE/COFF image version), > and defer loading the initrd contents until the point where the kernel > invokes the LoadFile2 protocol. > > Signed-off-by: Ard Biesheuvel > Signed-off-by: Nikita Ermakov > --- > grub-core/loader/arm64/linux.c | 117 ++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 1 deletion(-) > > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c > index aed7a200b..6b03455d1 100644 > --- a/grub-core/loader/arm64/linux.c > +++ b/grub-core/loader/arm64/linux.c > @@ -48,9 +48,18 @@ static grub_uint32_t cmdline_size; > static grub_addr_t initrd_start; > static grub_addr_t initrd_end; > > +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 }; > +static grub_efi_handle_t initrd_lf2_handle; > +static int initrd_use_loadfile2; Please explicitly initialize these two variables. > +static grub_efi_guid_t load_file2_guid = GRUB_EFI_LOAD_FILE2_PROTOCOL_GUID; > +static grub_efi_guid_t device_path_guid = GRUB_EFI_DEVICE_PATH_GUID; > + > grub_err_t > grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh) > { > + struct grub_pe32_coff_header *coff_header; > + struct grub_pe32_optional_header *optional_header; > + > if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE) > return grub_error(GRUB_ERR_BAD_OS, "invalid magic number"); > > @@ -61,6 +70,21 @@ grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh) > grub_dprintf ("linux", "UEFI stub kernel:\n"); > grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); > > + coff_header = (struct grub_pe32_coff_header *)((unsigned long)lh + lh->hdr_offset); coff_header = (struct grub_pe32_coff_header *) ((grub_addr_t) lh + lh->hdr_offset); ... please note missing spaces too. > + optional_header = (struct grub_pe32_optional_header *)(coff_header + 1); (struct grub_pe32_optional_header *) (coff_header + 1); > + /* > + * Linux kernels built for any architecture are guaranteed to support the > + * LoadFile2 based initrd loading protocol if the image version is >= 1. > + */ > + if (optional_header->major_image_version >= 1) > + initrd_use_loadfile2 = 1; > + else > + initrd_use_loadfile2 = 0; > + > + grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n", > + initrd_use_loadfile2 ? "en" : "dis"); > + > return GRUB_ERR_NONE; > } > > @@ -230,13 +254,86 @@ allocate_initrd_mem (int initrd_pages) > GRUB_EFI_LOADER_DATA); > } > > +struct initrd_media_device_path { > + grub_efi_vendor_media_device_path_t vendor; > + grub_efi_device_path_t end; > +} GRUB_PACKED; typedef struct initrd_media_device_path initrd_media_device_path_t; And please use initrd_media_device_path_t instead of "struct initrd_media_device_path". > + > +#define LINUX_EFI_INITRD_MEDIA_GUID \ > + { 0x5568e427, 0x68fc, 0x4f3d, \ > + { 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \ > + } Please move struct initrd_media_device_path and LINUX_EFI_INITRD_MEDIA_GUID definitions to the include/grub/efi/api.h file. > +static struct initrd_media_device_path initrd_lf2_device_path = { > + { > + { > + GRUB_EFI_MEDIA_DEVICE_PATH_TYPE, > + GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE, > + sizeof(grub_efi_vendor_media_device_path_t), > + }, > + LINUX_EFI_INITRD_MEDIA_GUID > + }, { > + GRUB_EFI_END_DEVICE_PATH_TYPE, > + GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE, > + sizeof(grub_efi_device_path_t) > + } > +}; This declaration/initialization should be behind device_path_guid declaration. > +static grub_efi_status_t > +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this, > + grub_efi_device_path_t *device_path, > + grub_efi_boolean_t boot_policy, > + grub_efi_uintn_t *buffer_size, > + void *buffer); Please drop this... > + > +static grub_efi_load_file2_t initrd_lf2 = { > + grub_efi_initrd_load_file2 > +}; ... and move this behind grub_efi_initrd_load_file2() definition. > +static grub_efi_status_t > +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this, > + grub_efi_device_path_t *device_path, > + grub_efi_boolean_t boot_policy, > + grub_efi_uintn_t *buffer_size, > + void *buffer) This works only because UEFI RISC-V/ARM and "normal" RISC-V/ARM calling conventions are the same. This will not work on x86_64. So, we have to introduce something like __grub_efi_api macro. It should be empty for at least for RISC-V and ARM but it should be "__attribute__ ((ms_abi))" for at least for x86_64. I think the __grub_efi_api macro should be defined in include/grub/efi/api.h next to efi_call_* definitions. > +{ > + grub_efi_status_t status = GRUB_EFI_SUCCESS; > + grub_efi_uintn_t initrd_size; > + > + if (!this || this != &initrd_lf2 || !buffer_size) if (this == NULL || this != &initrd_lf2 || buffer_size == NULL) Please use NULL explicitly below too... > + return GRUB_EFI_INVALID_PARAMETER; > + > + if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE || > + device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE) > + return GRUB_EFI_NOT_FOUND; > + > + if (boot_policy) > + return GRUB_EFI_UNSUPPORTED; > + > + initrd_size = grub_get_initrd_size (&initrd_ctx); > + if (!buffer || *buffer_size < initrd_size) > + { > + *buffer_size = initrd_size; > + return GRUB_EFI_BUFFER_TOO_SMALL; > + } > + > + grub_dprintf ("linux", "Providing initrd via LOAD_FILE2_PROTOCOL\n"); s/LOAD_FILE2_PROTOCOL/EFI_LOAD_FILE2_PROTOCOL/ > + if (grub_initrd_load (&initrd_ctx, buffer)) if (grub_initrd_load (&initrd_ctx, buffer) != GRUB_ERR_NONE)) > + status = GRUB_EFI_LOAD_ERROR; According to the UEFI spec this should be GRUB_EFI_DEVICE_ERROR. > + grub_initrd_close (&initrd_ctx); > + return status; > +} > + > static grub_err_t > grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > int argc, char *argv[]) > { > - struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 }; > int initrd_size, initrd_pages; > void *initrd_mem = NULL; > + grub_efi_boot_services_t *b; grub_efi_boot_services_t *b = grub_efi_system_table->boot_services; > + grub_efi_status_t status; > > if (argc == 0) > { > @@ -254,6 +351,24 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), > if (grub_initrd_init (argc, argv, &initrd_ctx)) > goto fail; > > + if (initrd_use_loadfile2 && !initrd_lf2_handle) > + { > + b = grub_efi_system_table->boot_services; ... then you can drop this. > + status = b->install_multiple_protocol_interfaces (&initrd_lf2_handle, > + &load_file2_guid, > + &initrd_lf2, > + &device_path_guid, > + &initrd_lf2_device_path, > + NULL); > + if (status == GRUB_EFI_OUT_OF_RESOURCES) > + { > + grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")); > + return grub_errno; s/return grub_errno/goto fail/ otherwise your are leaking memory. Or you can move protocols installation to the beginning of the function and return immediately in case of error. > + } I think we should not ignore the other UEFI errors here. > + grub_dprintf ("linux", "LoadFile2 initrd loading protocol installed\n"); > + return GRUB_ERR_NONE; > + } > + > initrd_size = grub_get_initrd_size (&initrd_ctx); > grub_dprintf ("linux", "Loading initrd\n"); Daniel