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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/04/16 22:26, Konrad Rzeszutek Wilk wrote:
> 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?

e_shoff needs checking to be within elf->len alone, before a calculation
involving e_shoff is checked against elf->len.

Specifically, if it were say (uint64_t)-1 in the elf header, then this
calculation would overflow and the check below would pass.

Similarly, something should check e_shentsize against an exact value,
given that its size is used to infer the layout of the section header
fields.

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

This is normal.  Traditionally, program headers live at the start of the
image, and section headers at the end.

The spec doesn't enforce this however.

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

Git grep says elf_resolve_sections() has

    if ( !elf->symtab->sec->sh_size ||
         elf->symtab->sec->sh_entsize < sizeof(Elf_Sym) )
    {
        dprintk(XENLOG_DEBUG, XSPLICE "%s: Symbol table header is
corrupted!\n",
                elf->name);
        return -EINVAL;
    }

I would check for !=, rather than <

Nothing good can come of having sh_entsize being bigger than what we
expect an Elf_Sym to be.

Also be aware that Elf_Sym.sh_entsize and Ehdr.e_shentsize appear to be
multiple locations containing the same information.  I would also cross
check them.

~Andrew

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