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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Apr 18, 2016 at 12:23:26AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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.

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

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;

(Or rather a combination of these and make the printk print the right warning).


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

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