Re: [Xen-devel] [PATCH v9 10/27] xsplice: Add helper elf routines

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.16 at 06:06, <konrad.wilk@xxxxxxxxxx> wrote:
>> > +static int xsplice_header_check(const struct xsplice_elf *elf)
>> > +{ 
> ..snip..
>> > +    if ( elf->hdr->e_shnum > 64 )
>> > +    {
>> > +        dprintk(XENLOG_ERR, XSPLICE "%s: Too many (%u) sections!\n",
>> > +                elf->name, elf->hdr->e_shnum);
>> > +        return -EOPNOTSUPP;
>> > +    }
>> > +
>> > +    if ( elf->hdr->e_shoff > ULONG_MAX )
>> 
>> Why not ">= elf->len" (and I see it was almost that way in v8.1)?
> 
> I misunderstood your comment. You mentioned to me that we have
> an boundary check here (when it was against elf->len) and that you
> wanted an overflow - so I replaced it - while you meant - in addition to.
> 
> But adding in both:
> 
>       elf->hdr->e_shoff >= ULONG_MAX || elf->hdr->e_shoff >= elf->len
> 
> feels unneccessary. And the boundary check is more imporant.
> I added both in the code.

And indeed the latter being more strict than the former, the former
should be dropped.

> v10:
>   - Change the check against 64 to be against SHN_LORESERVE

So we're moving between the extremes, and (as said in reply to v9)
I think we really want to be somewhere in the middle.

Andrew? Ross?

> +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 )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE"%s: Could not allocate memory for 
> section table!\n",
> +               elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    elf->sec = sec;
> +
> +    /* e_shoff and e_shnum overflow checks are done in xsplice_header_check. 
> */
> +    delta = elf->hdr->e_shoff + elf->hdr->e_shnum * elf->hdr->e_shentsize;
> +    if ( delta > elf->len )

You've added the suggested (transformation of the expression above)
check there, so the check here is now redundant and hence could be
dropped, or simply be converted to an ASSERT().

> +static int elf_resolve_section_names(struct xsplice_elf *elf, const void 
> *data)
> +{
> +    const char *shstrtab;
> +    unsigned int i;
> +    Elf_Off offset, delta;
> +    struct xsplice_elf_sec *sec;
> +    int rc;
> +
> +    /*
> +     * The elf->sec[0 -> e_shnum] structures have been verified by
> +     * elf_resolve_sections. Find file offset for section string table
> +     * (normally called .shstrtab)
> +     */
> +    sec = &elf->sec[elf->hdr->e_shstrndx];
> +
> +    rc = elf_verify_strtab(sec);
> +    if ( rc )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section string table is 
> corrupted\n",
> +                elf->name);
> +        return rc;
> +    }
> +
> +    /* Verified in elf_resolve_sections but just in case. */
> +    offset = sec->sec->sh_offset;
> +    ASSERT(offset < elf->len && (offset + sec->sec->sh_size <= elf->len));
> +
> +    shstrtab = data + offset;
> +
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        delta = elf->sec[i].sec->sh_name;
> +
> +        /* Boundary check on offset of name within the .shstrtab. */
> +        if ( delta >= sec->sec->sh_size )
> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end 
> of payload!\n",

You've fixed the message text in elf_get_sym() but not here.

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

Considering the different types of the expressions on both sides of
the ==, wouldn't it be better for offset to be of Elf_Off type?

> +    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;
> +
> +    sym = xmalloc_array(struct xsplice_elf_sym, nsym);
> +    if ( !sym )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Could not allocate memory for 
> symbols\n",
> +               elf->name);
> +        return -ENOMEM;
> +    }
> +
> +    /* So we don't leak memory. */
> +    elf->sym = sym;
> +
> +    for ( i = 1; i < nsym; i++ )
> +    {
> +        const Elf_Sym *s = symtab_sec->data + symtab_sec->sec->sh_entsize * 
> i;
> +
> +        delta = s->st_name;

And similarly here, for delta to be Elf_Word? Both more along the
lines of what elf_resolve_section_names() has...

> +static int xsplice_header_check(const struct xsplice_elf *elf)
> +{
> +    const Elf_Ehdr *hdr = elf->hdr;
> +
> +    if ( sizeof(*elf->hdr) > elf->len )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section header is bigger than 
> payload!\n",
> +                elf->name);
> +        return -EINVAL;
> +    }
> +
> +    if ( !IS_ELF(*hdr) )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Not an ELF payload!\n", elf->name);
> +        return -EINVAL;
> +    }
> +
> +    /* EI_CLASS, and e_flags are platform specific. */
> +    if ( hdr->e_version != EV_CURRENT ||
> +         hdr->e_ident[EI_VERSION] != EV_CURRENT ||
> +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||

As said, this also needs to become arch-specific.

> +         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||
> +         hdr->e_type != ET_REL ||
> +         hdr->e_phnum != 0 )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Invalid ELF payload!\n", elf->name);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    if ( elf->hdr->e_shstrndx == SHN_UNDEF )
> +    {
> +        dprintk(XENLOG_ERR, XSPLICE "%s: Section name idx is undefined!?\n",
> +                elf->name);
> +        return -EINVAL;
> +    }
> +
> +    /* Check that section name index is within the sections. */
> +    if ( elf->hdr->e_shstrndx >= elf->hdr->e_shnum )

Since this uses e_shnum as a boundary, it would seem more logical
for this to be done after the e_shnum check itself.

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

The message text is now stale (but may become correct again if the
conditional gets changed again).

> +                elf->name, elf->hdr->e_shnum);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    if ( elf->hdr->e_shoff >= elf->len || elf->hdr->e_shoff >= ULONG_MAX )

As said - the right side of the || is weaker than the left side, and
hence should be dropped.

Jan

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