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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/20/16 6:09 PM >>>
>On Mon, Apr 18, 2016 at 12:23:26AM -0600, Jan Beulich wrote:
>> >>> 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).
>
>OK, and re-reading above you say "imo cast away constness just there."
>so you are OK then with functions casting away constness if they
>need to modify the structure.

If it's really just one exceptional place where this happens, I guess it's okay 
as,
well, an exception. Otherwise the const would need to be dropped wherever
necessary to avoid having to cast it away in certain places.

>> >> >+ 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.
>
>I am not sure I understand you. Initially I thought you mean the
>p_filesz (which is the program header) - but this ELF file is not a
>program - it is an relocatable object so there are no program header.

Right, and I indeed mean the ELF header: The section headers clearly can't
start inside it.

>Ah, I think I mislead you! (with the 'if (!delta)..')The code will have:
>if ( !delta )
>return -EINVAL;
>if ( delta + sec[i].sec->sh_size > elf->len )
>return -EINVAL;

That's what I had understood, and the first if() is which I'd like to see made
slightly more strict.

>> >> >+ 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.
> 
>So earlier (in common/xsplice, verify_payload) there was a check on the
>size of the payload (up to 2MB), hence my check here would catch the
>overflow violation ((-1U) > 2(MB), but it is not very clear.
>
>I modified this above to be:
>
>303     if ( elf->hdr->e_shoff > ULONG_MAX - elf->hdr->e_shoff )               
>     

Something's clearly wrong here, with e_shoff used on both sides.

>304     {                                                                      
>     
>305         dprintk(XENLOG_ERR, XSPLICE "%s: Bogus e_shoff!\n", elf->name);    
>     
>306         return -EINVAL;                                                    
>     
>307     }                                                                      
>     
>308             
>
>I can also add back the  elf->hdr->e_shoff > elf->len in this
>conditional if you would like (along with the comment about 2(MB))

It's not because I like it, but for consistency with all the other elf->len 
checks.

Jan


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