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

[Alex Bennée]

Stefano Garzarella <address@hidden> writes:

> 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?
>>
>
> As Peter pointed out, maybe we should keep the 'goto fail' and do a
> better cleanup, but thanks to pointing that out to me.
>
>> Anyway:
>>
>> Reviewed-by: Alex Bennée <address@hidden>
>
> Thanks for the review!
>
> I'm sending a v2 following the Peter's suggestions,
> removing the error_report and returing a new ELF_LOAD_TOO_BIG error
> value.
>
> Can I keep your R-b, or would you like to have a look at v2?

I'm happy with that - I'm not super familiar with the Elf code although
I'm about to become so....

>
> Thanks,
> Stefano


--
Alex Bennée