Hi Ard,
On Mon, Jul 14, 2014 at 05:17:51PM +0100, Ard Biesheuvel wrote:
> The EFI stub for arm64 needs to behave like an ordinary bootloader in the sense
> that it needs to use the EFI environment and the Image header at runtime and
> not rely on the linker or preprocessor to produce values for text offset,
> image size and kernel size.
Could you elaborate on _why_ we can't do that, given it's linked into
the kernel Image?
Are we splitting the stub from the kernel? What's going on?
> This patch also fixes the corner case where Image happens to be loaded
> at exactly the right offset, but the allocation is actually too small
> to satisfy the requirement imposed by image_size as set in the header.
It's not really imposed by image_size. While it's described by
image_size it's imposed by the existence of the BSS section and the
initial page tables.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
> arch/arm64/kernel/Makefile | 2 --
> arch/arm64/kernel/efi-stub.c | 29 ++++++++++++++++-------------
> 2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index cdaedad3afe5..99b676eeeb0f 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -4,8 +4,6 @@
>
> CPPFLAGS_vmlinux.lds := -DTEXT_OFFSET=$(TEXT_OFFSET)
> AFLAGS_head.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> -CFLAGS_efi-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET) \
> - -I$(src)/../../../scripts/dtc/libfdt
>
> CFLAGS_REMOVE_ftrace.o = -pg
> CFLAGS_REMOVE_insn.o = -pg
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 9b61d66e2d20..4ba90b2ef677 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -11,8 +11,7 @@
> */
> #include <linux/efi.h>
> #include <asm/efi.h>
> -#include <asm/sections.h>
> -
> +#include <asm/image_hdr.h>
>
> efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> unsigned long *image_addr,
> @@ -23,24 +22,28 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
> efi_loaded_image_t *image)
> {
> efi_status_t status;
> - unsigned long kernel_size, kernel_memsize = 0;
> + struct arm64_image_hdr *hdr = (struct arm64_image_hdr *)*image_addr;
> +
> + /* make sure image_addr points to an arm64 kernel Image */
> + if (!arm64_image_hdr_check(hdr)) {
> + pr_efi_err(sys_table, "Kernel Image header check failed\n");
> + return EFI_LOAD_ERROR;
> + }
Surely this should always be the case if the stub is linked into the
kernel?
It would be nice to know the rationale for this.
>
> /* Relocate the image, if required. */
> - kernel_size = _edata - _text;
> - if (*image_addr != (dram_base + TEXT_OFFSET)) {
> - kernel_memsize = kernel_size + (_end - _edata) + TEXT_OFFSET;
> - status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
> + if (*image_addr != (dram_base + hdr->text_offset) ||
> + image->image_size < hdr->image_size) {
As far as I can tell the size of the Image (image->image_size) is always
going to be less than the effective run time image size
(hdr->image_size).
The SizeOfImage field in head.S which I assume image->image_size is
derived from (not having a UEFI spec in front of me) seems to cover
everything up to _edata but skips the BSS, and initial page tables, but
this is covered by the header's image_size field.
Am I missing something? Surely we _always_ expect image->image_size to
be smaller than hdr->image_size?
Cheers,
Mark.
> + *reserve_size = hdr->text_offset + hdr->image_size;
> + status = efi_low_alloc(sys_table, *reserve_size, SZ_2M,
> reserve_addr);
> if (status != EFI_SUCCESS) {
> pr_efi_err(sys_table, "Failed to relocate kernel\n");
> + *reserve_size = 0;
> return status;
> }
> - memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
> - kernel_size);
> - *image_addr = *reserve_addr + TEXT_OFFSET;
> - *reserve_size = kernel_memsize;
> + memcpy((void *)*reserve_addr + hdr->text_offset,
> + (void *)*image_addr, image->image_size);
> + *image_addr = *reserve_addr + hdr->text_offset;
> }
> -
> -
> return EFI_SUCCESS;
> }
> --
> 1.8.3.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
