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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/18/16 7:55 AM >>>
>> >+ 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.

Exactly. Yet what do you do above?

>> section header alone.
>
>I modify  sec[i].sec->sh_entsize in calc_section

See the comment there (in the next patch it was, I think).

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

As said, I'd suggest making this even more strict by checking at least against
the ELF header size.

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

I don't think so - there's no overflow being checked for here at all, this is
just a within bounds check. Just compare with the other one.

Jan


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