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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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

Yes. That ends up being checked in xsplice_header_check. Added comment
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

In earlier reviews you said that casting away constness in
function would be a big no no.

> section header alone.

I modify  sec[i].sec->sh_entsize in calc_section
> 
> >+ 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.

The loop (earlier) had started at 0 so the delta could have been zero.
Anyhow that not being the case anymore I will just do:

if ( !delta )
        return -EINVAL;
> 
> >+ /* 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?

We don't verify sh_name in elf_resolve_sections.

This is where we check that sh_name is within the shstrtab.

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

That one is stale. Removed the 'if ( delta)' part and the comment.
> 
> >+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().

This is the overflow check Andrew asked for.
> 
> >+struct xsplice_elf_sec {
> >+ Elf_Shdr *sec; /* Hooked up in elf_resolve_sections.*/
> 
> const
> 
> >+struct xsplice_elf_sym {
> >+ Elf_Sym *sym;
> 
> const

That will make 'xsplice_elf_resolve_symbols' unhappy:

elf->sym[i].sym->st_value = (unsigned
long)symbols_lookup_by_name(elf->sym[i].name);

Unless I cast away constness.
> 
> Jan

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