Re: [Xen-devel] [PATCH 12/22] libelf: Check pointer references in elf_is_elfbinary

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH 12/22] libelf: Check pointer references in 
elf_is_elfbinary"):
> On 07/06/13 19:27, Ian Jackson wrote:
> > @@ -332,11 +332,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct 
> > elf_binary *elf, ELF_HANDLE_DECL(
> >  
> >  /* 
> > ------------------------------------------------------------------------ */
> >  
> > -int elf_is_elfbinary(const void *image)
> > +int elf_is_elfbinary(const void *image_start, size_t image_size)
> >  {
> > -    const Elf32_Ehdr *ehdr = image;
> > +    const Elf32_Ehdr *ehdr = image_start;
> >  
> > -    return IS_ELF(*ehdr); /* fixme unchecked */
> > +    if ( image_size < sizeof(*ehdr) )
> > +        return 0;
> > +
> > +    return IS_ELF(*ehdr);
> >  }
> 
> sizeof(Elf32_Ehdr) == 52
> sizeof(Elf64_Ehdr) == 64
> 
> As stated by a trivial C program containing 'printf("32: %zu, 64:
> %zu\n", sizeof(Elf32_Ehdr), sizeof(Elf64_Ehdr));'
> 
> Therefore, the sizeof check is insufficient if given a 64bit elf.

The purpose of the size check here is to ensure that IS_ELF does not
compute or dereference a wild pointer.  It isn't intended to ensure
that the ELF actually has a valid header length.

I don't think that's necessary because this function's purpose is the
magic number check.  It doesn't promise that the ELF isn't malformed
somehow.  We need an explicit size check here because we call
elf_is_elfbinary /before/ setting up the elf_image and our
pointer-checking machinery; elf_is_elfbinary uses naked pointer
arithmetic so needs explicit checking.

Ian.

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