Re: [PATCH v2 07/13] xsplice: Add helper elf routines (v2)

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 01/14/2016 09:47 PM, 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.
>
> 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.
>      - Add checks for ELF file.
>      - Add name to be printed.
>      - Add len for easier ELF checks.
>      - Expand on the checks. Add macro.
> ---
> diff --git a/xen/common/xsplice_elf.c b/xen/common/xsplice_elf.c
> new file mode 100644
> index 0000000..a5e9d63
> --- /dev/null
> +++ b/xen/common/xsplice_elf.c
> @@ -0,0 +1,201 @@
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/xsplice.h>
> +#include <xen/xsplice_elf.h>
> +
> +#define return_(x) { printk(XENLOG_DEBUG "%s:%d rc: %d\n",  \
> +                            __func__,__LINE__, x); return x; }
> +
> +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);
> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR "Could not allocate memory for section table!\n");

Shouldn't this printk be removed if you're using return_?

> +        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 )
> +        {
> +                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);
> +        }
> +    }
> +    elf->sec = sec;
> +    if ( !elf->symtab )
> +        return_(-EINVAL);
> +
> +    /* There can be multiple SHT_STRTAB so pick the right one. */
> +    elf->strtab = &sec[elf->symtab->sec->sh_link];
> +
> +    if ( elf->symtab->sec->sh_size == 0 || elf->symtab->sec->sh_entsize == 0 )
> +        return_(-EINVAL);
> +
> +    if ( elf->symtab->sec->sh_entsize != sizeof(Elf_Sym) )
> +        return_(-EINVAL);
> +
> +    return 0;
> +}
> +
snip
> +
> +static int elf_get_sym(struct xsplice_elf *elf, uint8_t *data)
> +{
> +    struct xsplice_elf_sec *symtab_sec, *strtab_sec;
> +    struct xsplice_elf_sym *sym;
> +    unsigned int i, delta, offset;
> +
> +    symtab_sec = elf->symtab;
> +
> +    strtab_sec = elf->strtab;
> +
> +    /* Pointers arithmetic to get file offset. */
> +    offset = strtab_sec->data - data;
> +
> +    ASSERT( offset == strtab_sec->sec->sh_offset );
> +    /* symtab_sec->data was computed in elf_resolve_sections. */
> +    ASSERT((symtab_sec->sec->sh_offset + data) == symtab_sec->data );
> +
> +    /* No need to check values as elf_resolve_sections did it. */
> +    elf->nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
> +
> +    sym = xmalloc_array(struct xsplice_elf_sym, elf->nsym);
> +    if ( !sym )
> +    {
> +        printk(XENLOG_ERR "%s: Could not allocate memory for symbols\n", elf->name);

Shouldn't this printk be removed if you're using return_?

-- 
Ross Lagerwall