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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> 
> Add Elf routines and data structures in preparation for loading an
> xSplice payload.
> 
> We make an assumption that the max number of sections an ELF payload
> can have is 64. We can in future make this be dependent on the
> names of the sections and verifying against a list, but for right now
> this suffices.
> 
> Also we a whole lot of checks to make sure that the ELF payload
> file is not corrupted nor that the offsets point past the file.
> 
> For most of the checks we print an message if the hypervisor is built
> with debug enabled.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Reviewed-by: Andrew Cooper<andrew.cooper3@xxxxxxxxxx>

Again ...

> v9: Changed elf_verify_strtab to use const char and return EINVAL.
>     Remove 'if ( !delta )' check in elf_resolve_sections
>     Remove stale comments.
>     Fixed one off check against  sh_link.
>     Document boundary checks against shstrtab and symtab.
>     Fixed return codes in xsplice_header_check.
>     Add check for sections to not be within ELF header.
>     Added overflow check for e_shoff in xsplice_header_check.
>     Moved XSPLICE macro by four tabs.
>     Make ->sym be const.

... way too many changes for pre-existing tags to stay, at least
for my taste.

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

The added comment just helps make obvious that the overflow I
believe Andrew was worried about is still not being taken care of:
All xsplice_header_check() does is range check the two values
mentioned in the comment. But I agree that a proper range check
(at once eliminating overflow concerns for the arithmetic here)
would better live there (and also see there).

> +    if ( delta > elf->len )
> +    {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Section table is past end of 
> payload!\n",
> +                    elf->name);
> +            return -EINVAL;
> +    }
> +
> +    for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +    {
> +        delta = elf->hdr->e_shoff + i * elf->hdr->e_shentsize;
> +
> +        sec[i].sec = data + delta;
> +
> +        delta = sec[i].sec->sh_offset;
> +        /*
> +         * N.B. elf_resolve_section_names, elf_get_sym skip this check as
> +         * we do it here.
> +         */
> +        if ( delta < sizeof(Elf_Ehdr) ||
> +             (delta + sec[i].sec->sh_size > elf->len) )

The second half of the check needs to be skipped for SHT_NOBITS
sections. And beware of overflow again - both addends alone may
be too large, but the sum may be within range.

> +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;
> +
> +    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++ )
> +    {
> +        Elf_Sym *s = &((Elf_Sym *)symtab_sec->data)[i];

I'm sorry for not spotting this earlier, but the calculation here needs
to follow that of the section pointers into the section table, i.e. use
symtab_sec->sec->sh_entsize (which afaict at once will allow getting
rid of the cast, and which I guess will make obvious that this lacks a
const qualifier).

> +        delta = s->st_name;
> +        /* Boundary check within the .strtab. */
> +        if ( delta > strtab_sec->sec->sh_size )

>= (just like in elf_resolve_section_names())

> +        {
> +            dprintk(XENLOG_ERR, XSPLICE "%s: Symbol [%u] data is past end of 
> payload!\n",

Message text does not match context (also in
elf_resolve_section_names() as I now see).

> +                    elf->name, i);
> +            return -EINVAL;
> +        }
> +
> +        sym[i].sym = s;
> +        sym[i].name = data + (delta + offset);

I think this

        sym[i].name = strtab_sec->data + delta;

would be more obvious to the reader.

> +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;
> +    }
> +
> +    if ( hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
> +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
> +         hdr->e_ident[EI_OSABI] != ELFOSABI_SYSV ||

What about EI_VERSION and EI_ABIVERSION, btw?

> +         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 )
> +    {
> +        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 > 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)?
And then followed (further down) by another check taking
elf->hdr->e_shnum * elf->hdr->e_shentsize into account (of
course as things stand now, elf->hdr->e_shentsize can also be
arbitrarily large, so this would need to be suitably structured
- e.g. "(elf->len - elf->hdr->e_shoff) / elf->hdr->e_shentsize <
elf->hdr->e_shnum").

Jan

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