Hi Philipp,

On Mon, Jul 01, 2019 at 02:31:20PM +0200, Philipp Rudo wrote:
> Sven Schnelle <sv...@stackframe.org> wrote:
> 
> > I'm attaching the patch to this Mail. What do you think about that change?
> > s390 also uses ELF files, and (maybe?) could also switch to this 
> > implementation.
> > But i don't know anything about S/390 and don't have one in my basement. So
> > i'll leave s390 to the IBM folks.
> 
> I'm afraid there isn't much code here s390 can reuse. I see multiple problems
> in kexec_elf_load:
> 
> * while loading the phdrs we also need to setup some data structures to pass
>   to the next kernel
> * the s390 kernel needs to be loaded to a fixed address
> * there is no support to load a crash kernel
> 
> Of course that could all be fixed/worked around by introducing some arch 
> hooks.
> But when you take into account that the whole elf loader on s390 is ~100 lines
> of code, I don't think it is worth it.

That's fine. I didn't really look into the S/390 Loader, and just wanted to let
the IBM people know.

> More comments below.
>  
> [...]
> 
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index b9b1bc5f9669..49b23b425f84 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -216,6 +216,41 @@ extern int crash_prepare_elf64_headers(struct 
> > crash_mem *mem, int kernel_map,
> >                                    void **addr, unsigned long *sz);
> >  #endif /* CONFIG_KEXEC_FILE */
> >  
> > +#ifdef CONFIG_KEXEC_FILE_ELF
> > +
> > +struct kexec_elf_info {
> > +   /*
> > +    * Where the ELF binary contents are kept.
> > +    * Memory managed by the user of the struct.
> > +    */
> > +   const char *buffer;
> > +
> > +   const struct elfhdr *ehdr;
> > +   const struct elf_phdr *proghdrs;
> > +   struct elf_shdr *sechdrs;
> > +};
> 
> Do i understand this right? elf_info->buffer contains the full elf file and
> elf_info->ehdr is a (endianness translated) copy of the files ehdr?
> 
> If so ...
> 
> > +void kexec_free_elf_info(struct kexec_elf_info *elf_info);
> > +
> > +int kexec_build_elf_info(const char *buf, size_t len, struct elfhdr *ehdr,
> > +                     struct kexec_elf_info *elf_info);
> > +
> > +int kexec_elf_kernel_load(struct kimage *image, struct kexec_buf *kbuf,
> > +                     char *kernel_buf, unsigned long kernel_len,
> > +                     unsigned long *kernel_load_addr);
> > +
> > +int kexec_elf_probe(const char *buf, unsigned long len);
> > +
> > +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> > +                    struct kexec_elf_info *elf_info,
> > +                    struct kexec_buf *kbuf,
> > +                    unsigned long *lowest_load_addr);
> > +
> > +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> > +                    struct kexec_elf_info *elf_info,
> > +                    struct kexec_buf *kbuf,
> > +                    unsigned long *lowest_load_addr);
> 
> ... you could simplify the arguments by dropping the ehdr argument. The
> elf_info should contain all the information needed. Furthermore the kexec_buf
> also contains a pointer to its kimage. So you can drop that argument as well.
> 
> An other thing is that you kzalloc the memory needed for proghdrs and sechdrs
> but expect the user of those functions to provide the memory needed for ehdr.
> Wouldn't it be more consistent to also kzalloc the ehdr?
> 

Good point. I'll think about it. I would like to do that in an extra patch,
as it is not a small style change.

> 
> > diff --git a/kernel/kexec_file_elf.c b/kernel/kexec_file_elf.c
> > new file mode 100644
> > index 000000000000..bb966c93492c
> > --- /dev/null
> > +++ b/kernel/kexec_file_elf.c
> > @@ -0,0 +1,574 @@
> 
> [...]
> 
> > +static uint64_t elf64_to_cpu(const struct elfhdr *ehdr, uint64_t value)
> > +{
> > +   if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> > +           value = le64_to_cpu(value);
> > +   else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> > +           value = be64_to_cpu(value);
> > +
> > +   return value;
> > +}
> > +
> > +static uint16_t elf16_to_cpu(const struct elfhdr *ehdr, uint16_t value)
> > +{
> > +   if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> > +           value = le16_to_cpu(value);
> > +   else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> > +           value = be16_to_cpu(value);
> > +
> > +   return value;
> > +}
> > +
> > +static uint32_t elf32_to_cpu(const struct elfhdr *ehdr, uint32_t value)
> > +{
> > +   if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> > +           value = le32_to_cpu(value);
> > +   else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> > +           value = be32_to_cpu(value);
> > +
> > +   return value;
> > +}
> 
> What are the elf*_to_cpu good for? In general I'd assume that kexec loads a
> kernel for the same architecture it is running on. So the new kernel should
> also have the same endianness like the one which loads it. Is this a
> ppcle/ppcbe issue?

Don't know. I would agree, but i'm not an powerpc expert.

> Furthermore the current order is 64->16->32, which my OCPD absolutely hates :)

Fixed.

> [...]
> 
> > +/**
> > + * elf_is_shdr_sane - check that it is safe to use the section header
> > + * @buf_len:       size of the buffer in which the ELF file is loaded.
> > + */
> > +static bool elf_is_shdr_sane(const struct elf_shdr *shdr, size_t buf_len)
> > +{
> > +   bool size_ok;
> > +
> > +   /* SHT_NULL headers have undefined values, so we can't check them. */
> > +   if (shdr->sh_type == SHT_NULL)
> > +           return true;
> > +
> > +   /* Now verify sh_entsize */
> > +   switch (shdr->sh_type) {
> > +   case SHT_SYMTAB:
> > +           size_ok = shdr->sh_entsize == sizeof(Elf_Sym);
> > +           break;
> > +   case SHT_RELA:
> > +           size_ok = shdr->sh_entsize == sizeof(Elf_Rela);
> > +           break;
> > +   case SHT_DYNAMIC:
> > +           size_ok = shdr->sh_entsize == sizeof(Elf_Dyn);
> > +           break;
> > +   case SHT_REL:
> > +           size_ok = shdr->sh_entsize == sizeof(Elf_Rel);
> > +           break;
> > +   case SHT_NOTE:
> > +   case SHT_PROGBITS:
> > +   case SHT_HASH:
> > +   case SHT_NOBITS:
> > +   default:
> > +           /*
> > +            * This is a section whose entsize requirements
> > +            * I don't care about.  If I don't know about
> > +            * the section I can't care about it's entsize
> > +            * requirements.
> > +            */
> > +           size_ok = true;
> > +           break;
> > +   }
> > +
> > +   if (!size_ok) {
> > +           pr_debug("ELF section with wrong entry size.\n");
> > +           return false;
> > +   } else if (shdr->sh_addr + shdr->sh_size < shdr->sh_addr) {
> > +           pr_debug("ELF section address wraps around.\n");
> > +           return false;
> > +   }
> > +
> > +   if (shdr->sh_type != SHT_NOBITS) {
> > +           if (shdr->sh_offset + shdr->sh_size < shdr->sh_offset) {
> > +                   pr_debug("ELF section location wraps around.\n");
> > +                   return false;
> > +           } else if (shdr->sh_offset + shdr->sh_size > buf_len) {
> > +                   pr_debug("ELF section not in file.\n");
> > +                   return false;
> > +           }
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +static int elf_read_shdr(const char *buf, size_t len, struct 
> > kexec_elf_info *elf_info,
> > +                    int idx)
> > +{
> > +   struct elf_shdr *shdr = &elf_info->sechdrs[idx];
> > +   const struct elfhdr *ehdr = elf_info->ehdr;
> > +   const char *sbuf;
> > +   struct elf_shdr *buf_shdr;
> > +
> > +   sbuf = buf + ehdr->e_shoff + idx * sizeof(*buf_shdr);
> > +   buf_shdr = (struct elf_shdr *) sbuf;
> > +
> > +   shdr->sh_name      = elf32_to_cpu(ehdr, buf_shdr->sh_name);
> > +   shdr->sh_type      = elf32_to_cpu(ehdr, buf_shdr->sh_type);
> > +   shdr->sh_addr      = elf_addr_to_cpu(ehdr, buf_shdr->sh_addr);
> > +   shdr->sh_offset    = elf_addr_to_cpu(ehdr, buf_shdr->sh_offset);
> > +   shdr->sh_link      = elf32_to_cpu(ehdr, buf_shdr->sh_link);
> > +   shdr->sh_info      = elf32_to_cpu(ehdr, buf_shdr->sh_info);
> > +
> > +   /*
> > +    * The following fields have a type equivalent to Elf_Addr
> > +    * both in 32 bit and 64 bit ELF.
> > +    */
> > +   shdr->sh_flags     = elf_addr_to_cpu(ehdr, buf_shdr->sh_flags);
> > +   shdr->sh_size      = elf_addr_to_cpu(ehdr, buf_shdr->sh_size);
> > +   shdr->sh_addralign = elf_addr_to_cpu(ehdr, buf_shdr->sh_addralign);
> > +   shdr->sh_entsize   = elf_addr_to_cpu(ehdr, buf_shdr->sh_entsize);
> > +
> > +   return elf_is_shdr_sane(shdr, len) ? 0 : -ENOEXEC;
> > +}
> > +
> > +/**
> > + * elf_read_shdrs - read the section headers from the buffer
> > + *
> > + * This function assumes that the section header table was checked for 
> > sanity.
> > + * Use elf_is_ehdr_sane() if it wasn't.
> > + */
> > +static int elf_read_shdrs(const char *buf, size_t len,
> > +                     struct kexec_elf_info *elf_info)
> > +{
> > +   size_t shdr_size, i;
> > +
> > +   /*
> > +    * e_shnum is at most 65536 so calculating
> > +    * the size of the section header cannot overflow.
> > +    */
> > +   shdr_size = sizeof(struct elf_shdr) * elf_info->ehdr->e_shnum;
> > +
> > +   elf_info->sechdrs = kzalloc(shdr_size, GFP_KERNEL);
> > +   if (!elf_info->sechdrs)
> > +           return -ENOMEM;
> > +
> > +   for (i = 0; i < elf_info->ehdr->e_shnum; i++) {
> > +           int ret;
> > +
> > +           ret = elf_read_shdr(buf, len, elf_info, i);
> > +           if (ret) {
> > +                   kfree(elf_info->sechdrs);
> > +                   elf_info->sechdrs = NULL;
> > +                   return ret;
> > +           }
> > +   }
> > +
> > +   return 0;
> > +}
> 
> In the end you only use the phdrs. So in theory you can drop everything shdr
> related. Although keeping it would be 'formally more correct'.

Correct, done.
 
> [...]
> 
> > +/**
> > + * kexec_free_elf_info - free memory allocated by elf_read_from_buffer
> > + */
> > +void kexec_free_elf_info(struct kexec_elf_info *elf_info)
> > +{
> > +   kfree(elf_info->proghdrs);
> > +   kfree(elf_info->sechdrs);
> > +   memset(elf_info, 0, sizeof(*elf_info));
> > +}
> > +EXPORT_SYMBOL(kexec_free_elf_info);
> 
> Why are you exporting these functions? Is there any kexec implementation out
> there which is put into a module? Do you even want that to be possible?

My fault. Fixed.

> > +/**
> > + * kexec_build_elf_info - read ELF executable and check that we can use it
> > + */
> > +int kexec_build_elf_info(const char *buf, size_t len, struct elfhdr *ehdr,
> > +                     struct kexec_elf_info *elf_info)
> > +{
> > +   int i;
> > +   int ret;
> > +
> > +   ret = elf_read_from_buffer(buf, len, ehdr, elf_info);
> > +   if (ret)
> > +           return ret;
> > +
> > +   /* Big endian vmlinux has type ET_DYN. */
> > +   if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN) {
> 
> s390 is big endian and it's vmlinux has type ET_EXEC. So I assume that this is
> a ppc issue?

Again, don't know. :)

> > +           pr_err("Not an ELF executable.\n");
> > +           goto error;
> > +   } else if (!elf_info->proghdrs) {
> > +           pr_err("No ELF program header.\n");
> > +           goto error;
> > +   }
> > +
> > +   for (i = 0; i < ehdr->e_phnum; i++) {
> > +           /*
> > +            * Kexec does not support loading interpreters.
> > +            * In addition this check keeps us from attempting
> > +            * to kexec ordinay executables.
> > +            */
> > +           if (elf_info->proghdrs[i].p_type == PT_INTERP) {
> > +                   pr_err("Requires an ELF interpreter.\n");
> > +                   goto error;
> > +           }
> > +   }
> > +
> > +   return 0;
> > +error:
> > +   kexec_free_elf_info(elf_info);
> > +   return -ENOEXEC;
> > +}
> > +EXPORT_SYMBOL(kexec_build_elf_info);
> 
> [...]
> 
> > +int kexec_elf_probe(const char *buf, unsigned long len)
> > +{
> > +   struct elfhdr ehdr;
> > +   struct kexec_elf_info elf_info;
> > +   int ret;
> > +
> > +   ret = kexec_build_elf_info(buf, len, &ehdr, &elf_info);
> 
> On s390 I only check the elf magic when probing. That's because the image
> loader cannot reliably check the image and thus accepts everything that is
> given to it. That also means that any elf file the elf probe rejects (e.g.
> because it has a phdr with type PT_INTERP) is passed on to the image loader,
> which happily takes it.
> 
> If you plan to also add an image loader you should keep that in mind.
> 
> Thanks
> Philipp
> 

Reply via email to