[Skiboot] [RFC PATCH 5/6] core/init: Move calculating elf size out of try_load_elf64{_le, }()
Jordan Niethe
jniethe5 at gmail.com
Thu Dec 19 10:33:31 AEDT 2019
On Wed, Dec 18, 2019 at 7:19 PM Oliver O'Halloran <oohall at gmail.com> wrote:
>
> 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;
That is a good idea.
>
> > +}
> > +
> > 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.
I would like to, but I was not sure if we wanted to care about the case when
no kernel-base-address was given in the device tree, but we are still
using a preloaded
kernel at KERNEL_LOAD_BASE.
That is what I left the calculations in for.
>
> > + 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