Re: [PATCH v1 06/11] xsplice: Add helper elf routines

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 03, 2015 at 06:16:03PM +0000, Ross Lagerwall wrote:
> Add some elf routines and data structures in preparation for loading an
> xsplice payload.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/common/Makefile           |   1 +
>  xen/common/xsplice_elf.c      | 122 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/xsplice_elf.h |  44 +++++++++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 xen/common/xsplice_elf.c
>  create mode 100644 xen/include/xen/xsplice_elf.h
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 1b17c9d..de7c08a 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -57,6 +57,7 @@ obj-y += vsprintf.o
>  obj-y += wait.o
>  obj-y += xmalloc_tlsf.o
>  obj-y += xsplice.o
> +obj-y += xsplice_elf.o
>  
>  obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
>  
> diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
> new file mode 100644
> index 0000000..13a9229
> --- /dev/null
> +++ b/xen/common/xsplice_elf.c
> @@ -0,0 +1,122 @@

Do you want to add your company copyright header here?

> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/xsplice_elf.h>
> +
> +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf,
> +                                                const char *name)
> +{
> +    int i;

unsigned int ?

> +
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( !strcmp(name, elf->sec[i].name) )
> +            return &elf->sec[i];
> +    }
> +
> +    return NULL;
> +}
> +
> +static int elf_get_sections(struct xsplice_elf *elf, uint8_t *data)
> +{
> +    struct xsplice_elf_sec *sec;
> +    int i;

unsigned int;

Should we check the e_shnum for out of bound values? Hmm,
uint16t so not that bad..

> +
> +    sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum);
> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR "Could not find section table\n");

Well that is not exactly right. It couldnt' allocate the memory.

Perhaps we should say:
	"Could not allocate memory for %s which has %u sections!\n", elf->name, elf->hdr->e_shnum);

> +        return -ENOMEM;
> +    }
> +
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +#ifdef CONFIG_ARM_32
> +        sec[i].sec = (Elf32_Shdr *)(data + elf->hdr->e_shoff +
> +                                    i * elf->hdr->e_shentsize);
> +#else
> +        sec[i].sec = (Elf64_Shdr *)(data + elf->hdr->e_shoff +
> +                                    i * elf->hdr->e_shentsize);
> +#endif

> +        sec[i].data = data + sec[i].sec->sh_offset;

We should validate that the 'sec[i].data' pointers are not outside
the memory allocated for 'data'.

> +    }
> +    elf->sec = sec;
> +
> +    return 0;
> +}
> +
> +static int elf_get_sym(struct xsplice_elf *elf, uint8_t *data)
> +{
> +    struct xsplice_elf_sec *symtab, *strtab_sec;
> +    struct xsplice_elf_sym *sym;
> +    const char *strtab;
> +    int i;
> +
> +    symtab = xsplice_elf_sec_by_name(elf, ".symtab");
> +    if ( !symtab )
> +    {
> +        printk(XENLOG_ERR "Could not find symbol table\n");
> +        return -EINVAL;
> +    }
> +
> +    strtab_sec = xsplice_elf_sec_by_name(elf, ".strtab");
> +    if ( !strtab_sec )
> +    {
> +        printk(XENLOG_ERR "Could not find string table\n");
> +        return -EINVAL;
> +    }
> +    strtab = (const char *)(data + strtab_sec->sec->sh_offset);

Should we do a check to make sure that 'strtab' is not bigger
than the amount of memory allocated for 'data'?

> +
> +    elf->nsym = symtab->sec->sh_size / symtab->sec->sh_entsize;
> +
> +    sym = xmalloc_array(struct xsplice_elf_sym, elf->nsym);

Perhaps also a validity check on the elf->nsym?..
> +    if ( !sym )
> +    {
> +        printk(XENLOG_ERR "Could not allocate memory for symbols\n");
> +        return -ENOMEM;
> +    }
> +
> +    for ( i = 0; i < elf->nsym; i++ )
> +    {
> +#ifdef CONFIG_ARM_32
> +        sym[i].sym = (Elf32_Sym *)(symtab->data + i * symtab->sec->sh_entsize);
> +#else
> +        sym[i].sym = (Elf64_Sym *)(symtab->data + i * symtab->sec->sh_entsize);
> +#endif
> +        sym[i].name = strtab + sym[i].sym->st_name;

Could we check that the 'sym[i].name is not outside the memory allocated
for data' ?
> +    }
> +    elf->sym = sym;
> +
> +    return 0;
> +}
> +
> +int xsplice_elf_load(struct xsplice_elf *elf, uint8_t *data, ssize_t len)
> +{
> +    const char *shstrtab;
> +    int i, rc;

unsigned int i;

> +
> +#ifdef CONFIG_ARM_32
> +    elf->hdr = (Elf32_Ehdr *)data;
> +#else
> +    elf->hdr = (Elf64_Ehdr *)data;
> +#endif
> +
> +    rc = elf_get_sections(elf, data);
> +    if ( rc )
> +        return rc;
> +
> +    shstrtab = (const char *)(data + elf->sec[elf->hdr->e_shstrndx].sec->sh_offset);

if (shstrtab > data + len)
	return -EINVAL;

> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +        elf->sec[i].name = shstrtab + elf->sec[i].sec->sh_name;
> +
> +    rc = elf_get_sym(elf, data);
> +    if ( rc )
> +        return rc;
> +
> +    return 0;
> +}
> +
> +void xsplice_elf_free(struct xsplice_elf *elf)
> +{
> +    xfree(elf->sec);
> +    xfree(elf->sym);

elf->sec = NULL;
elf->sym = NULL

just in case..
> +}
> diff --git a/xen/include/xen/xsplice_elf.h b/xen/include/xen/xsplice_elf.h
> new file mode 100644
> index 0000000..bac0053
> --- /dev/null
> +++ b/xen/include/xen/xsplice_elf.h
> @@ -0,0 +1,44 @@
> +#ifndef __XEN_XSPLICE_ELF_H__
> +#define __XEN_XSPLICE_ELF_H__
> +
> +#include <xen/types.h>
> +#include <xen/elfstructs.h>
> +
> +/* The following describes an Elf file as consumed by xsplice. */
> +struct xsplice_elf_sec {
> +#ifdef CONFIG_ARM_32
> +    Elf32_Shdr *sec;
> +#else
> +    Elf64_Shdr *sec;
> +#endif
> +    const char *name;
> +    const uint8_t *data;           /* A pointer to the data section */
> +    uint8_t *load_addr;            /* A pointer to the allocated destination */

Missing full stop.
> +};
> +
> +struct xsplice_elf_sym {
> +#ifdef CONFIG_ARM_32
> +    Elf32_Sym *sym;
> +#else
> +    Elf64_Sym *sym;
> +#endif
> +    const char *name;
> +};
> +
> +struct xsplice_elf {
> +#ifdef CONFIG_ARM_32
> +    Elf32_Ehdr *hdr;
> +#else
> +    Elf64_Ehdr *hdr;
> +#endif
> +    struct xsplice_elf_sec *sec;   /* Array of sections */
> +    struct xsplice_elf_sym *sym;   /* Array of symbols */

Missing full stop.
> +    int nsym;

unsigned int nsym;
> +};
> +
> +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf,
> +                                                const char *name);
> +int xsplice_elf_load(struct xsplice_elf *elf, uint8_t *data, ssize_t len);
> +void xsplice_elf_free(struct xsplice_elf *elf);
> +
> +#endif /* __XEN_XSPLICE_ELF_H__ */
> -- 
> 2.4.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel