Re: [Xen-devel] [PATCH v5 09/28] xsplice: Add helper elf routines

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/common/xsplice_elf.c
> @@ -0,0 +1,294 @@
> +/*
> + * Copyright (C) 2016 Citrix Systems R&D Ltd.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/xsplice_elf.h>
> +#include <xen/xsplice.h>
> +
> +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++ )

Unless you do something unusual (which would be undesirable),
useful ELF section number start from 1.

> +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data)
> +{
> +    struct xsplice_elf_sec *sec;
> +    unsigned int i;
> +
> +    /* xsplice_elf_load sanity checked e_shnum checked. */

redundant "checked"

> +    sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum);
> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR "%s%s: Could not allocate memory for section 
> table!\n",
> +               XSPLICE, elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    elf->sec = sec;
> +
> +    /* N.B. We also will ingest SHN_UNDEF sections. */

Because of? The meaning of the fields in the 0-th section header is
different from that of ordinary ones.

> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        ssize_t delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;

Why ssize_t? (This anyway should be a suitable ELF type.)

> +
> +        if ( delta + sizeof(Elf_Shdr) > elf->len )
> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Section header [%d] is past end of 
> payload!\n",
> +                    XSPLICE, elf->name, i);

XSPLICE is a string literal, so should be prepended to the format
string instead of forced through %s. And %u please for unsigned
arguments.

Also this check doesn't need doing inside the loop - you can simply
check once (using e_shnum) that the entire section table is valid.

> +            return -EINVAL;
> +        }
> +
> +        sec[i].sec = (Elf_Shdr *)(data + delta);

Pointless cast bogusly casting away constness.

> +        delta = sec[i].sec->sh_offset;
> +
> +        if ( delta > elf->len )

This is relevant only for sections having non-zero size. And then you of
course need to take size into account when dong the bounds check.

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Section [%d] data is past end of 
> payload!\n",
> +                    XSPLICE, elf->name, i);
> +            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 )
> +            {
> +                dprintk(XENLOG_DEBUG, "%s%s: Multiple symbol tables!\n",
> +                        XSPLICE, elf->name);
> +                return -EINVAL;

There's nothing invalid about this, it's simply unsupported by the
implementation (read: a better error code please).

> +            }
> +
> +            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 )

>=

> +            {
> +                dprintk(XENLOG_DEBUG, "%s%s: Symbol table idx (%d) to strtab 
> past end (%d)\n",
> +                        XSPLICE, elf->name, elf->symtab->sec->sh_link,
> +                        elf->hdr->e_shnum);
> +                return -EINVAL;
> +            }
> +        }
> +    }
> +
> +    if ( !elf->symtab )
> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: No symbol table found!\n",
> +                XSPLICE, elf->name);
> +        return -EINVAL;
> +    }
> +
> +    /* There can be multiple SHT_STRTAB so pick the right one. */
> +    elf->strtab = &sec[elf->symtab->sec->sh_link];

How about checking this really is a SHT_STRTAB section?

> +    if ( !elf->symtab->sec->sh_size || !elf->symtab->sec->sh_entsize ||
> +         elf->symtab->sec->sh_entsize != sizeof(Elf_Sym) )

The first sh_entsize check is redundant with the second, and the
second is too strict (< suffices).

Also shouldn't the string table section also have at least non-zero
size? And its first and last bytes be NUL?

> +static int elf_resolve_section_names(struct xsplice_elf *elf, const void 
> *data)
> +{
> +    const char *shstrtab;
> +    unsigned int i;
> +    unsigned int offset, delta;
> +
> +    /*
> +     * The elf->sec[0 -> e_shnum] structures have been verified by
> +     * elf_resolve_sections. Find file offset for section string table.
> +     */
> +    offset =  elf->sec[elf->hdr->e_shstrndx].sec->sh_offset;

Truncating the value on 64-bit ELF.

> +    if ( offset > elf->len )
> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: shstrtab section offset (%u) past end 
> of payload!\n",
> +                XSPLICE, elf->name, elf->hdr->e_shstrndx);
> +        return -EINVAL;
> +    }
> +
> +    shstrtab = (data + offset);

Pointless parentheses.

> +    /* We could ignore the first as it is reserved.. */

Double full stop.

> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        delta = elf->sec[i].sec->sh_name;
> +
> +        if ( offset + delta > elf->len )

This is too weak: After having bounds checked the string table section
to be inside the image, you now need to bounds check the string offset
to be inside the string table. Also it seems (just like above) you
no-where check that the referenced section actually is a string table.

> +static int elf_get_sym(struct xsplice_elf *elf, const void *data)
> +{
> +    struct xsplice_elf_sec *symtab_sec, *strtab_sec;
> +    struct xsplice_elf_sym *sym;
> +    unsigned int i, delta, offset, nsym;
> +
> +    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. */
> +    nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
> +
> +    sym = xmalloc_array(struct xsplice_elf_sym, nsym);
> +    if ( !sym )
> +    {
> +        printk(XENLOG_ERR "%s%s: Could not allocate memory for symbols\n",
> +               XSPLICE, elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    /* So we don't leak memory. */
> +    elf->sym = sym;
> +    for ( i = 0; i < nsym; i++ )

As with sections, the 0th symbol table entry is special too.

> +    {
> +        Elf_Sym *s;
> +
> +        if ( i * sizeof(Elf_Sym) > elf->len )

Considering that we know the symbol table section is within bounds,
I don't think this check does any good. Plus it ought to be adding 1
to i and take the section file offset into account.

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Symbol header [%d] is past end of  
> payload!\n",
> +                    XSPLICE, elf->name, i);
> +            return -EINVAL;
> +        }
> +
> +        s = &((Elf_Sym *)symtab_sec->data)[i];
> +
> +        /* If st->name is STN_UNDEF is zero, the check will always be true. 
> */

Odd double use of "is".

> +        delta = s->st_name;
> +
> +        /* Offset has been computed earlier. */
> +        if ( offset + delta > elf->len )

This should again check against the string table size and again use >= .

> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Symbol [%u] data is past end of 
> payload!\n",
> +                    XSPLICE, elf->name, i);
> +            return -EINVAL;
> +        }
> +
> +        sym[i].sym = s;
> +        if ( s->st_name == STN_UNDEF )
> +            sym[i].name = NULL;

I don't think this is a good idea - since the first byte of a string table
needs to be NUL, you can as well just point there (without any need
for a conditional).

> +        else
> +            sym[i].name = data + ( delta + offset );

Stray blanks.

> +static int xsplice_header_check(const struct xsplice_elf *elf)
> +{
> +    if ( sizeof(*elf->hdr) >= elf->len )

Strictly speaking just > .

> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: Section header is bigger than 
> payload!\n",
> +                XSPLICE, elf->name);
> +        return -EINVAL;
> +    }
> +
> +    if ( elf->hdr->e_shstrndx == SHN_UNDEF )
> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: Section name idx is undefined!?\n",
> +                XSPLICE, elf->name);
> +        return -EINVAL;
> +    }
> +
> +    /* Check that section name index is within the sections. */
> +    if ( elf->hdr->e_shstrndx > elf->hdr->e_shnum )

>=

> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: Section name idx (%d) is past end of  
> sections (%d)!\n",
> +                XSPLICE, elf->name, elf->hdr->e_shstrndx, elf->hdr->e_shnum);
> +        return -EINVAL;
> +    }
> +
> +    if ( elf->hdr->e_shnum > 64 )
> +    {
> +        dprintk(XENLOG_DEBUG, "%s%s: Too many (%d) sections!\n",
> +                XSPLICE, elf->name, elf->hdr->e_shnum);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}

Assuming there are no further checks hidden in other patches, I'm
afraid there's quite a bit of stuff missing - there are plenty of other
header fields most if not all of which need sanity checking.

> +int xsplice_elf_load(struct xsplice_elf *elf, void *data)
> +{
> +    int rc;
> +
> +    elf->hdr = data;
> +
> +    rc = xsplice_header_check(elf);
> +    if ( rc )
> +        return rc;
> +
> +    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;
> +}

Instead of zeroing these fields, wouldn't it make sense to simply
xfree(elf) as the last action here?

> --- /dev/null
> +++ b/xen/include/xen/xsplice_elf.h
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (C) 2016 Citrix Systems R&D Ltd.
> + */
> +
> +#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?

> +    const char *name;              /* Human readable name hooked in
> +                                      elf_resolve_section_names. */
> +    const void *data;              /* Pointer to the section (done by
> +                                      elf_resolve_sections). */
> +};
> +
> +struct xsplice_elf_sym {
> +    Elf_Sym *sym;

const?

> +    const char *name;
> +};
> +
> +struct xsplice_elf {
> +    const char *name;              /* Pointer to payload->name. */
> +    ssize_t len;                   /* Length of the ELF file. */

Why ssize_t?

> +    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]. */

Many times - const?

> +};
> +
> +struct xsplice_elf_sec *xsplice_elf_sec_by_name(const struct xsplice_elf 
> *elf,
> +                                                const char *name);

const (return value)?

> +int xsplice_elf_load(struct xsplice_elf *elf, void *data);

const (second parameter)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel