[Skiboot] [RFC PATCH 5/6] core/init: Move calculating elf size out of try_load_elf64{_le, }()
Oliver O'Halloran
oohall at gmail.com
Wed Dec 18 19:19:39 AEDT 2019
On Wed, Dec 18, 2019 at 3:52 PM Jordan Niethe <jniethe5 at gmail.com> wrote:
>
> At the end of try_load_elf64() and try_load_elf64_le() kernel_size
> is set to the size of the elf. This size is calculated by assuming:
> 1) the elf has sections, and
> 2) the section header is at the end of the elf.
>
> These assumptions are not assured by the ELF specification. If the elf
Pick one capitalisation *whipcrack*
> does not match the assumptions, an incorrect size will be logged.
> Further this is confusing as it means that at various times kernel_size
> either refers to the size of the enter payload or just that of the elf.
> So remove this calculation from here.
>
> It is still necessary to call secureboot_verify() and
> trustedboot_measure() on Mambo, so move this size calculation there.
> Later a more reliable way of determining the size can be introduced.
>
> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> ---
> core/init.c | 63 ++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/core/init.c b/core/init.c
> index 96c770f03476..58e99c6163c1 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -125,12 +125,8 @@ static bool try_load_elf64_le(struct elf_hdr *header)
> kernel_entry += load_base;
> kernel_32bit = false;
>
> - kernel_size = le64_to_cpu(kh->e_shoff) +
> - ((uint32_t)le16_to_cpu(kh->e_shentsize) *
> - (uint32_t)le16_to_cpu(kh->e_shnum));
> -
> - prlog(PR_DEBUG, "INIT: 64-bit kernel entry at 0x%llx, size 0x%lx\n",
> - kernel_entry, kernel_size);
> + prlog(PR_DEBUG, "INIT: 64-bit kernel entry at 0x%llx\n",
> + kernel_entry);
>
> return true;
> }
> @@ -209,11 +205,7 @@ static bool try_load_elf64(struct elf_hdr *header)
> kernel_entry += load_base;
> kernel_32bit = false;
>
> - kernel_size = kh->e_shoff +
> - ((uint32_t)kh->e_shentsize * (uint32_t)kh->e_shnum);
> -
> - printf("INIT: 64-bit kernel entry at 0x%llx, size 0x%lx\n",
> - kernel_entry, kernel_size);
> + printf("INIT: 64-bit kernel entry at 0x%llx\n", kernel_entry);
>
> return true;
> }
> @@ -317,6 +309,32 @@ static bool try_load_elf32(struct elf_hdr *header)
> return true;
> }
>
> +static uint64_t elf_size(struct elf_hdr *h)
> +{
> + struct elf64_hdr *h64 = (struct elf64_hdr *)h;
> + struct elf32_hdr *h32 = (struct elf32_hdr *)h;
> + uint64_t size;
> +
> + if (h->ei_class == ELF_CLASS_64 && h->ei_data == ELF_DATA_LSB) {
> + size = le64_to_cpu(h64->e_shoff) +
> + ((uint32_t)le16_to_cpu(h64->e_shentsize) *
> + (uint32_t)le16_to_cpu(h64->e_shnum));
> + } else if (h->ei_class == ELF_CLASS_64 && h->ei_data == ELF_DATA_MSB) {
> + size = h64->e_shoff + ((uint32_t)h64->e_shentsize *
> + (uint32_t)h64->e_shnum);
> + } else if (h->ei_class == ELF_CLASS_32 && h->ei_data == ELF_DATA_LSB) {
> + size = le32_to_cpu(h32->e_shoff) +
> + ((uint32_t)le16_to_cpu(h64->e_shentsize) *
> + (uint32_t)le16_to_cpu(h64->e_shnum));
> + } else if (h->ei_class == ELF_CLASS_32 && h->ei_data == ELF_DATA_MSB) {
> + size = h32->e_shoff + ((uint32_t)h32->e_shentsize *
> + (uint32_t)h32->e_shnum);
You should be using the beXX_to_cpu() accessors for the BE cases too
since Nick's trying to make Skiboot LE.
> + } else {
> + size = 0; /* XXX */
Make it an assert I guess?
> + }
> + return size;
We could avoid all the casting if we do something like this for each case:
if (h->ei_class == ELF_CLASS_64 && h->ei_data == ELF_DATA_LSB) {
sh_offset = <blah>
sh_entsz = <blah>
sh_num = <blah>
} else if (h->ei_class == ELF_CLASS_64 && h->ei_data == ELF_DATA_MSB) {
...
} /* you get the idea */
and at the end:
return sh_offset + sh_entsz * sh_num;
> +}
> +
> extern char __builtin_kernel_start[];
> extern char __builtin_kernel_end[];
> extern uint64_t boot_offset;
> @@ -355,6 +373,7 @@ bool start_preload_kernel(void)
> static bool load_kernel(void)
> {
> void *stb_container = NULL;
> + uint64_t stb_size;
> struct elf_hdr *kh;
> int loaded;
>
> @@ -435,12 +454,22 @@ static bool load_kernel(void)
> }
>
> if (chip_quirk(QUIRK_MAMBO_CALLOUTS)) {
> - secureboot_verify(RESOURCE_ID_KERNEL,
> - stb_container,
> - SECURE_BOOT_HEADERS_SIZE + kernel_size);
> - trustedboot_measure(RESOURCE_ID_KERNEL,
> - stb_container,
> - SECURE_BOOT_HEADERS_SIZE + kernel_size);
> + /*
> + * A kernel_size of 0 means an unknown sized preloaded kernel.
> + * The size is needed for testing STB, so estimate the size of
> + * the elf from its headers. This calculation makes assumptions
> + * about the structure of the elf that are not guaranteed by
> + * the spec, but it is good enough for here.
> + */
Can we get rid of the size calculations entirely? This is mambo-only
logic and skiboot.tcl now provides us the size explicitly.
> + if (kernel_size)
> + stb_size = kernel_size;
> + else
> + stb_size = elf_size(kh) + SECURE_BOOT_HEADERS_SIZE;
> +
> + secureboot_verify(RESOURCE_ID_KERNEL, stb_container,
> + stb_size);
> + trustedboot_measure(RESOURCE_ID_KERNEL, stb_container,
> + stb_size);
> }
>
> return true;
> --
> 2.17.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list