Re: [Xen-devel] [PATCH 15/21] libelf: check loops for running away

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("Re: [Xen-devel] [PATCH 15/21] libelf: check loops for 
running away"):
> On Wed, Jun 12, 2013 at 5:00 PM, Ian Jackson <ian.jackson@xxxxxxxxxxxxx> 
> wrote:

> > +        /*
> > +         * This test also arranges for the loop to terminate if the
> > +         * input file has a ridiculous value for the header count: The
> > +         * first putative header outside the input image will appear
> > +         * to have type 0 (since out-of-range accesses read as 0) and
> > +         * PT_NOTE != 0.
> > +         */
> >          if ( elf_uval(elf, phdr, p_type) != PT_NOTE )
> >              continue;
> 
> Sorry, run that by me again?  It looks like it will call "continue" in
> that case, not "break".
> 
> This looks inconsistent with the changes you made in libelf-loader.c
> and xc_dom_elfloader.c: There we have similar types of reads with
> continues, but you do an access check anyway.
> 
> Is there something special about phdr vs shdr?

You are entirely correct.  I must have misread "continue" as "break".

> > +            /*
> > +             * See above re guarantee of loop termination.
> > +             * SHT_NOTE != 0.
> > +             */
> >              if ( elf_uval(elf, shdr, sh_type) != SHT_NOTE )
> >                  continue;
> 
> Same as above.

Again.

I will fix these by changing them to use the same checks as elsewhere.

> > @@ -306,7 +323,14 @@ ELF_HANDLE_DECL(elf_note) elf_note_next(struct 
> > elf_binary *elf, ELF_HANDLE_DECL(
> >      unsigned namesz = (elf_uval(elf, note, namesz) + 3) & ~3;
> >      unsigned descsz = (elf_uval(elf, note, descsz) + 3) & ~3;
> >
> > -    return ELF_MAKE_HANDLE(elf_note, ELF_HANDLE_PTRVAL(note) + 
> > elf_size(elf, note) + namesz + descsz);
> > +    elf_ptrval ptrval = ELF_HANDLE_PTRVAL(note)
> > +        + elf_size(elf, note) + namesz + descsz;
> > +
> > +    if ( ( ptrval <= ELF_HANDLE_PTRVAL(note) || /* wrapped or stuck */
> > +           !elf_access_ok(elf, ELF_HANDLE_PTRVAL(note), 1) ) )
> > +        ptrval = ELF_MAX_PTRVAL; /* terminate caller's loop */
> 
> Is this an official part of the interface, or do you just "know" that
> the two callers will happen to break out of their loop when this
> happens?
> 
> If it's the second, this seems like a future bug waiting to happen...

I will add a comment, making it part of the declared interface.

Ian.

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