Re: [PATCH v6 08/24] xsplice: Add helper elf routines

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

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data)
> +{
> +    struct xsplice_elf_sec *sec;
> +    unsigned int i;
> +    Elf_Off delta;
> +    int rc;
> +
> +    /* xsplice_elf_load sanity checked e_shnum. */
> +    sec = xmalloc_array(struct xsplice_elf_sec, elf->hdr->e_shnum);
> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR XSPLICE"%s: Could not allocate memory for section table!\n",
> +               elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    elf->sec = sec;
> +
> +    delta = elf->hdr->e_shoff + elf->hdr->e_shnum * elf->hdr->e_shentsize;

Have we verified any of these to be sane yet?  (i.e. what about
calculation overflow?)

(Edit: e_shnum yes, e_shentsize and e_shoff look to be no)

> +    if ( delta >= elf->len )
> +    {
> +            dprintk(XENLOG_DEBUG, XSPLICE "%s: Section table is past end of payload!\n",
> +                    elf->name);
> +            return -EINVAL;
> +    }

(Mis)-alignment


> +static int elf_get_sym(struct xsplice_elf *elf, const void *data)
> +{
> +    const 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;
> +
> +    /* Checked already in elf_resolve_sections, but just in case. */
> +    ASSERT(offset == strtab_sec->sec->sh_offset);
> +    ASSERT(offset < elf->len && (offset + strtab_sec->sec->sh_size <= elf->len));
> +
> +    /* 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;

Has anything checked sh_entsize for being 0 or -1 ?

Being unsigned, -1 cant happen, but nothing checks got being nonzero.

With these things fixed, Reviewed-by: Andrew
Cooper<andrew.cooper3@citrix.com>

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