diff mbox

Load ELF64 binaries correctly

Message ID 54C7D22C.5040202@freebsd.org
State Superseded
Headers show

Commit Message

Nathan Whitehorn Jan. 27, 2015, 6 p.m. UTC
On 01/27/15 09:59, Nathan Whitehorn wrote:
> On 01/22/15 09:02, Benjamin Herrenschmidt wrote:
>> On Wed, 2015-01-21 at 08:45 -0800, Nathan Whitehorn wrote:
>>> The attached patch fixes the big-endian ELF64 loader in skiboot to
>>> handle the fact that the ELF entry point is specified to point to a
>>> function descriptor describing the entry point rather than the entry
>>> point itself. (I haven't set it up to load the TOC base pointer though)
>>> This is required to load the FreeBSD kernel as a skiboot payload. The
>>> patch does not touch the little-endian loader since I'm not sure if the
>>> ELFv2 spec still has function descriptors or not.
>> An additional problem is that the Linux 64-bit BE kernel vmlinux doesn't
>> have a correct function descriptor as an entry point :-( So that patch
>> breaks loading a raw BE vmlinux... We probably need a quirk to recognize
>> such an image, possibly via the fact that the value that would be in the
>> function descriptor cannot possibly be a valid entry point as part of
>> the image.
>
> Here's a second version of the patch that can load both Linux and 
> FreeBSD. It iterates through the ELF section table and, if the entry 
> point points to an executable section, treats the entry point as the 
> first instruction to run. Otherwise, it treats it as a function 
> descriptor.
> -Nathan

Sorry, missed a file in the patch. Here's the right one.
-Nathan

Comments

Benjamin Herrenschmidt Jan. 27, 2015, 9:06 p.m. UTC | #1
On Tue, 2015-01-27 at 10:00 -0800, Nathan Whitehorn wrote:
> On 01/27/15 09:59, Nathan Whitehorn wrote:
> > On 01/22/15 09:02, Benjamin Herrenschmidt wrote:
> >> On Wed, 2015-01-21 at 08:45 -0800, Nathan Whitehorn wrote:
> >>> The attached patch fixes the big-endian ELF64 loader in skiboot to
> >>> handle the fact that the ELF entry point is specified to point to a
> >>> function descriptor describing the entry point rather than the entry
> >>> point itself. (I haven't set it up to load the TOC base pointer though)
> >>> This is required to load the FreeBSD kernel as a skiboot payload. The
> >>> patch does not touch the little-endian loader since I'm not sure if the
> >>> ELFv2 spec still has function descriptors or not.
> >> An additional problem is that the Linux 64-bit BE kernel vmlinux doesn't
> >> have a correct function descriptor as an entry point :-( So that patch
> >> breaks loading a raw BE vmlinux... We probably need a quirk to recognize
> >> such an image, possibly via the fact that the value that would be in the
> >> function descriptor cannot possibly be a valid entry point as part of
> >> the image.
> >
> > Here's a second version of the patch that can load both Linux and 
> > FreeBSD. It iterates through the ELF section table and, if the entry 
> > point points to an executable section, treats the entry point as the 
> > first instruction to run. Otherwise, it treats it as a function 
> > descriptor.
> > -Nathan
> 
> Sorry, missed a file in the patch. Here's the right one.

I would have gone for something even simpler such as checking the
boundary of the descriptor :-) But what you did is probably cleaner.

Can you re-submit in the appropriate format ? I'm not sure we've
documented it yet but we follow the same practices as Linux here that 
is a subject, a changeset comment, a Signed-off-by line, then the patch
(the important thing for us is the S-O-B without which we cannot apply
it).

Cheers,
Ben.
 
> -Nathan
Stewart Smith Jan. 28, 2015, 12:58 a.m. UTC | #2
Nathan Whitehorn <nwhitehorn@freebsd.org> writes:
> On 01/27/15 09:59, Nathan Whitehorn wrote:
>> On 01/22/15 09:02, Benjamin Herrenschmidt wrote:
>>> On Wed, 2015-01-21 at 08:45 -0800, Nathan Whitehorn wrote:
>>>> The attached patch fixes the big-endian ELF64 loader in skiboot to
>>>> handle the fact that the ELF entry point is specified to point to a
>>>> function descriptor describing the entry point rather than the entry
>>>> point itself. (I haven't set it up to load the TOC base pointer though)
>>>> This is required to load the FreeBSD kernel as a skiboot payload. The
>>>> patch does not touch the little-endian loader since I'm not sure if the
>>>> ELFv2 spec still has function descriptors or not.
>>> An additional problem is that the Linux 64-bit BE kernel vmlinux doesn't
>>> have a correct function descriptor as an entry point :-( So that patch
>>> breaks loading a raw BE vmlinux... We probably need a quirk to recognize
>>> such an image, possibly via the fact that the value that would be in the
>>> function descriptor cannot possibly be a valid entry point as part of
>>> the image.
>>
>> Here's a second version of the patch that can load both Linux and 
>> FreeBSD. It iterates through the ELF section table and, if the entry 
>> point points to an executable section, treats the entry point as the 
>> first instruction to run. Otherwise, it treats it as a function 
>> descriptor.

Cool. I'll let Ben do some ack/review on it.

It'd be great if you could resend with a Signed-off-by line (we follow
Signed-off-by etc just like linux kernel does), that'd be great!
Stewart Smith Jan. 28, 2015, 1:25 a.m. UTC | #3
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> Can you re-submit in the appropriate format ? I'm not sure we've
> documented it yet but we follow the same practices as Linux here that 
> is a subject, a changeset comment, a Signed-off-by line, then the patch
> (the important thing for us is the S-O-B without which we cannot apply
> it).

I now mention it in the README and I just point to the linux
Documentation/SubmittingPatches rather than having spent any time
documenting it separately.

We possibly should... and I wonder if we should co-ordinate somewhat
with other OpenPower related projects so that we get to a consistent way
of working.
diff mbox

Patch

diff --git a/core/init.c b/core/init.c
index 2c7e30c..e75b5a3 100644
--- a/core/init.c
+++ b/core/init.c
@@ -111,6 +111,7 @@  static bool try_load_elf64(struct elf_hdr *header)
 	struct elf64_hdr *kh = (struct elf64_hdr *)header;
 	uint64_t load_base = (uint64_t)kh;
 	struct elf64_phdr *ph;
+	struct elf64_shdr *sh;
 	unsigned int i;
 
 	/* Check it's a ppc64 LE ELF */
@@ -152,6 +153,27 @@  static bool try_load_elf64(struct elf_hdr *header)
 		prerror("INIT: Failed to find kernel entry !\n");
 		return false;
 	}
+
+	/* For the normal big-endian ELF ABI, the kernel entry points
+	 * to a function descriptor in the data section. Linux instead
+	 * has it point directly to code. Test whether it is pointing
+	 * into an executable section or not to figure this out. Default
+	 * to assuming it obeys the ABI.
+	 */
+	sh = (struct elf64_shdr *)(load_base + kh->e_shoff);
+	for (i = 0; i < kh->e_shnum; i++, sh++) {
+		if (sh->sh_addr > kh->e_entry ||
+		    (sh->sh_addr + sh->sh_size) <= kh->e_entry)
+			continue;
+
+		break;
+	}
+
+	if (i == kh->e_shnum || !(sh->sh_flags & ELF_SFLAGS_X)) {
+		kernel_entry = *(uint64_t *)(kernel_entry + load_base);
+		kernel_entry = kernel_entry - ph->p_vaddr + ph->p_offset;
+	}
+
 	kernel_entry += load_base;
 	kernel_32bit = false;
 
diff --git a/include/elf.h b/include/elf.h
index 0a52f3e..c600f7f 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -76,6 +76,23 @@  struct elf64_phdr {
 	uint64_t p_align;
 };
 
+/* 64-bit ELF section header */
+struct elf64_shdr {
+	uint32_t sh_name;
+	uint32_t sh_type;
+	uint64_t sh_flags;
+#define ELF_SFLAGS_X	0x4
+#define ELF_SFLAGS_A	0x2
+#define ELF_SFLAGS_W	0x1
+	uint64_t sh_addr;
+	uint64_t sh_offset;
+	uint64_t sh_size;
+	uint32_t sh_link;
+	uint32_t sh_info;
+	uint64_t sh_addralign;
+	uint64_t sh_entsize;
+};
+
 /* Some relocation related stuff used in relocate.c */
 struct elf64_dyn {
 	int64_t	 d_tag;