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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:03 AM >>>
>+static int elf_verify_strtab(const struct xsplice_elf_sec *sec)
>+{
>+ const Elf_Shdr *s;
>+ const uint8_t *contents;

Considering it's a string table, perhaps better const char *?

>+ s = sec->sec;
>+
>+ if ( s->sh_type != SHT_STRTAB )
>+ return -EINVAL;
>+
>+ if ( !s->sh_size )
>+ return -EOPNOTSUPP;

Why not also -EINVAL? I don't think the standard permits empty string table
sections.

>+ contents = (const uint8_t *)sec->data;

Pointless cast.

>+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;
>+
>+ delta = elf->hdr->e_shoff + elf->hdr->e_shnum * elf->hdr->e_shentsize;

I think Andrew had asked this before - are we not worried of overflow here?

>+ 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 = (void *)data + delta;

Casting away constness is one of the main reasons I complain about casts in
general. If you really need to modify some fields in the section header, you 
should
imo cast away constness just there. But even better would be if you left the
section header alone.

>+ 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 && (delta + sec[i].sec->sh_size > elf->len) )

Allowing delta to be zero here seems suspicious: Are you doing that because
some sections come without data (and hence without offset)? In that case you'd
better skip the check on those section types (e.g. SHT_NOBITS) only. In fact
the offset being below the end of the ELF header would likely indicate a broken
image.

>+ /* Name is populated in xsplice_elf_sections_name. */

Stale function name afaict.

>+ /*
>+ * 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 )

>= (and I know I had asked before to check all these bounds checks for off
by one errors)

>+ if ( !elf->symtab->sec->sh_size ||
>+ elf->symtab->sec->sh_entsize < sizeof(Elf_Sym) )

|| sh_size % sh_entsize

>+ /*
>+ * There can be multiple SHT_STRTAB (.shstrtab, .strtab) so pick one
>+ * associated with the symbol table.
>+ */

... pick the one ...

>+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;
>+
>+ if ( delta && delta >= sec->sec->sh_size )
>+ {
>+ dprintk(XENLOG_ERR, XSPLICE "%s: shstrtab [%u] data is past end of 
>payload!\n",
>+ elf->name, i);

Isn't this redundant with the check in elf_resolve_sections()? Or is this needed
because the one here runs before that other one?

>+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];
>+
>+ /* If st->name is STN_UNDEF zero, the check will always be true. */
>+ delta = s->st_name;
>+
>+ if ( delta && (delta > strtab_sec->sec->sh_size) )

Now here I don't see why you special case delta being zero, the more with the
comment right above.

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

This isn't invalid, but unsupported.

>+ if ( elf->hdr->e_shoff > elf->len )
>+ {
>+ dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);
>+ return -EINVAL;
>+ }

If this check is needed, then >=. But I think it should be redundant with the 
more
complete one in elf_resolve_sections().

>+struct xsplice_elf_sec {
>+ Elf_Shdr *sec; /* Hooked up in elf_resolve_sections.*/

const

>+struct xsplice_elf_sym {
>+ Elf_Sym *sym;

const

Jan

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