Re: [Qemu-devel] [PATCH] elf-ops.h: fix int overflow in load_elf()

[Daniel P . Berrangé]

On Tue, Sep 10, 2019 at 10:50:30AM +0100, Alex Bennée wrote:
> 
> Stefano Garzarella <address@hidden> writes:
> 
> > This patch fixes a possible integer overflow when we calculate
> > the total size of ELF segments loaded.
> >
> > Reported-by: Coverity (CID 1405299)
> > Signed-off-by: Stefano Garzarella <address@hidden>
> > ---
> > Now we are limited to INT_MAX, should load_elf() returns ssize_t
> > to support bigger ELFs?
> > ---
> >  include/hw/elf_ops.h | 6 ++++++
> >  hw/core/loader.c     | 1 +
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> > index 1496d7e753..46dd3bf413 100644
> > --- a/include/hw/elf_ops.h
> > +++ b/include/hw/elf_ops.h
> > @@ -485,6 +485,12 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> >                  }
> >              }
> >
> > +            if (mem_size > INT_MAX - total_size) {
> > +                error_report("ELF total segments size is too big to load "
> > +                             "max is %d)", INT_MAX);
> > +                goto fail;
> > +            }
> > +
> 
> Seem sensible enough (although gah, I hate these glue bits). Would the
> large amount of goto fail logic be something that could be cleaned up
> with the automatic cleanup functions we recently mentioned in
> CODING_STYLE.rst?

The target has this:

 fail:
    g_mapped_file_unref(mapped_file);
    g_free(phdr);
    return ret;



GMappedFIle supports the  g_autoptr cleanup, and g_free is trivially
done with g_autofree.

So yes, you can entirely kill the goto's in this function using
auto cleanup

> 
> Anyway:
> 
> Reviewed-by: Alex Bennée <address@hidden>
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|