Re: [PATCH v3 05/23] xsplice: Add helper elf routines (v4)

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/16 18:05, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Add Elf routines and data structures in preparation for loading an
> xSplice payload.
>
> We also add an macro that will print where we failed during
> the ELF parsing - which is only available during debug builds.
> In production (debug=n) we only return the error value.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v2: - With the #define ELFSIZE in the ARM file we can use the common
>      #defines instead of using #ifdef CONFIG_ARM_32. Moved to another
>     patch.
>     - Add checks for ELF file.
>     - Add name to be printed.
>     - Add len for easier ELF checks.
>     - Expand on the checks. Add macro.
> v3: Remove the return_ macro
> v4: Add return_ macro back but make it depend on debug=y
> ---
>  xen/common/Makefile           |   1 +
>  xen/common/xsplice_elf.c      | 205 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/xsplice_elf.h |  37 ++++++++
>  3 files changed, 243 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 43b3911..a8ceaff 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -74,3 +74,4 @@ subdir-y += libelf
>  subdir-$(CONFIG_HAS_DEVICE_TREE) += libfdt
>  
>  obj-$(CONFIG_XSPLICE) += xsplice.o
> +obj-$(CONFIG_XSPLICE) += xsplice_elf.o

Again, location in the makefile.

> diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
> new file mode 100644
> index 0000000..d9f9002
> --- /dev/null
> +++ b/xen/common/xsplice_elf.c
> @@ -0,0 +1,205 @@
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/xsplice_elf.h>
> +#include <xen/xsplice.h>
> +
> +#ifdef NDEBUG
> +#define return_(x) return x
> +#else
> +#define return_(x) { printk(XENLOG_DEBUG "%s:%d rc: %d\n",  \
> +                            __func__,__LINE__, x); return x; }

:(  This is a horrible antipattern.  Just use dprintk() which gets you
this information anyway, and allows for more textural information.

> +#endif
> +
> +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf *elf,
> +                                                const char *name)
> +{
> +    unsigned int i;
> +
> +    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_resolve_sections(struct xsplice_elf *elf, uint8_t *data)
> +{
> +    struct xsplice_elf_sec *sec;
> +    unsigned int i;
> +
> +    sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum);

Presumably there will be some sanity checks done somewhere between the
hypercall and here?

> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR "Could not allocate memory for section table!\n");
> +        return_(-ENOMEM);
> +    }
> +
> +    /* N.B. We also will ingest SHN_UNDEF sections. */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        ssize_t delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
> +
> +        if ( delta + sizeof(Elf_Shdr) > elf->len )
> +            return_(-EINVAL);
> +
> +        sec[i].sec = (Elf_Shdr *)(data + delta);
> +        delta = sec[i].sec->sh_offset;
> +
> +        if ( delta > elf->len )
> +            return_(-EINVAL);
> +
> +        sec[i].data = data + delta;
> +        /* Name is populated in xsplice_elf_sections_name. */
> +        sec[i].name = NULL;
> +
> +        if ( sec[i].sec->sh_type == SHT_SYMTAB )
> +        {

(mis) alignment.

> +                if ( elf->symtab )
> +                    return_(-EINVAL);
> +                elf->symtab = &sec[i];
> +                /* elf->symtab->sec->sh_link would point to the right section
> +                 * but we hadn't finished parsing all the sections. */
> +                if ( elf->symtab->sec->sh_link > elf->hdr->e_shnum )
> +                    return_(-EINVAL);
> +        }
> +    }

Newline please.

> <snip>
> +
> +int xsplice_elf_load(struct xsplice_elf *elf, uint8_t *data)
> +{
> +    int rc;
> +
> +    elf->hdr = (Elf_Ehdr *)data;

A lot of this code would be neater (i.e. without explicit typecasts) if
data was void * rather than uint8_t.  GCC pointer arithmetic on void *
works in a sane way.

> +
> +    if ( sizeof(*elf->hdr) >= elf->len )
> +        return_(-EINVAL);
> +
> +    if ( elf->hdr->e_shstrndx == SHN_UNDEF )
> +        return_(-EINVAL);
> +
> +    /* Check that section name index is within the sections. */
> +    if ( elf->hdr->e_shstrndx > elf->hdr->e_shnum )
> +        return_(-EINVAL);
> +
> +    rc = elf_resolve_sections(elf, data);
> +    if ( rc )
> +        return rc;
> +
> +    rc = elf_resolve_section_names(elf, data);
> +    if ( rc )
> +        return rc;
> +
> +    rc = elf_get_sym(elf, data);
> +    if ( rc )
> +        return rc;
> +
> +    return 0;
> +}
> +
> +void xsplice_elf_free(struct xsplice_elf *elf)
> +{
> +    xfree(elf->sec);
> +    elf->sec = NULL;
> +    xfree(elf->sym);
> +    elf->sym = NULL;
> +    elf->nsym = 0;
> +    elf->name = NULL;
> +    elf->len = 0;
> +}

Variable block please.

> diff --git a/xen/include/xen/xsplice_elf.h b/xen/include/xen/xsplice_elf.h
> new file mode 100644
> index 0000000..42dbc6f
> --- /dev/null
> +++ b/xen/include/xen/xsplice_elf.h
> @@ -0,0 +1,37 @@
> +#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 {
> +    Elf_Shdr *sec;                 /* Hooked up in elf_resolve_sections. */
> +    const char *name;              /* Human readable name hooked in
> +                                      elf_resolve_section_names. */
> +    const uint8_t *data;           /* Pointer to the section (done by
> +                                      elf_resolve_sections). */
> +};
> +
> +struct xsplice_elf_sym {
> +    Elf_Sym *sym;
> +    const char *name;
> +};
> +
> +struct xsplice_elf {
> +    const char *name;              /* Pointer to payload->name. */
> +    ssize_t len;                   /* Length of the ELF file. */
> +    Elf_Ehdr *hdr;                 /* ELF file. */
> +    struct xsplice_elf_sec *sec;   /* Array of sections, allocated by us. */
> +    struct xsplice_elf_sym *sym;   /* Array of symbols , allocated by us. */
> +    unsigned int nsym;
> +    struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka to sec[x]. */
> +    struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka to sec[y]. */
> +};
> +
> +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);
> +void xsplice_elf_free(struct xsplice_elf *elf);
> +
> +#endif /* __XEN_XSPLICE_ELF_H__ */

Variable block please.

~Andrew