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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Apr 08, 2016 at 03:53:44PM +0100, Andrew Cooper wrote:
> 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)

e_shentsize is uint16_t
e_shoff is uint64_t or uint32_t.

Where you think a check against UINT_MAX/ULONG_MAX for the e_shoff?

> 
> > +    if ( delta >= elf->len )

This should have been >

As I found out some linkers are happy to place that whole section table
at the end of the file. Which means that this checks gets triggered.
> > +    {
> > +            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 ?

Let me double-check.
> 
> Being unsigned, -1 cant happen, but nothing checks got being nonzero.
> 
> With these things fixed, Reviewed-by: Andrew
> Cooper<andrew.cooper3@xxxxxxxxxx>

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