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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

> > +static int elf_resolve_sections(struct xsplice_elf *elf, const void *data)
> > +{
.. snip..
> > +    /* 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++ )

The reason for this is not obvious .. In the payload loading patch I
iterate over each elf->sec (starting at zero) and immediately
dereference the sh_type:
        if ( (elf->sec[i].sec->sh_flags .. )

As you can imagine if I don't set elf->sec[0].sec this blows up. Hence
the odd start at zero.

However one can as well just fix the loop in 'move_payload' to start
at 1 instead of 0 - which is what I did.

> > +    {
> > +        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 >= .

I reworked this a bit (borrowed your idea of checking the full size of
the section before the loop) - which removes the need to check
the offset.

What I ended up is something much simpler (as I know the offset
is OK - I just need to check that the delta is within the section):
        if ( delta && (delta > strtab_sec->sec->sec_sh_size) )
                ..

The offset gets (in the new patchset) checked in elf_resolve_section.

Albeit I am not sure about the >= instead of >, .. I need to think of
that.

.. snip..
> > +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?

The  struct xsplice_elf is allocated on the stack (in the next
patch).

> > --- /dev/null
> > +++ b/xen/include/xen/xsplice_elf.h
.. snip..
> > +struct xsplice_elf_sym {
> > +    Elf_Sym *sym;
> 
> const?

.. this is much harder. I end up computing the values for
these symbols and have to write to this this structure a couple of times
(at worst).
> 
> > +    const char *name;
> > +};
> > +
> > +struct xsplice_elf {
> > +    const char *name;              /* Pointer to payload->name. */
> > +    ssize_t len;                   /* Length of the ELF file. */
> 
> Why ssize_t?

Made it 'size_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?

I have made the symtab and strtab const, but the 'sec' and 'sym'
I can't easily. There are many instances where I poke in the
section (like for ELF relocations) and have to modify this.

I can do some casting but it gets a bit .. messy.

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