include/hw/elf_ops.h | 6 ++++++ hw/core/loader.c | 1 + 2 files changed, 7 insertions(+)
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 <sgarzare@redhat.com>
---
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;
+ }
+
/* address_offset is hack for kernel images that are
linked at the wrong physical address. */
if (translate_fn) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 32f7cc7c33..feda52fa88 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -44,6 +44,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
+#include "qemu/error-report.h"
#include "qapi/error.h"
#include "hw/hw.h"
#include "disas/disas.h"
--
2.21.0
On Tue, 10 Sep 2019 at 10:08, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> 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 <sgarzare@redhat.com>
> ---
> 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;
> + }
This function doesn't report issues via error_report()
(some callers intentionally have fallback options for
what they try to do with the file), but by returning
a suitable error value in 'ret', so I think we should
continue that approach rather than adding an error_report()
call here.
I agree that accumulating the size in an 'int' is a bit
dubious these days.
thanks
-- PMM
On Tue, Sep 10, 2019 at 10:37:28AM +0100, Peter Maydell wrote:
> On Tue, 10 Sep 2019 at 10:08, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > 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 <sgarzare@redhat.com>
> > ---
> > 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;
> > + }
>
> This function doesn't report issues via error_report()
> (some callers intentionally have fallback options for
> what they try to do with the file), but by returning
> a suitable error value in 'ret', so I think we should
> continue that approach rather than adding an error_report()
> call here.
I agree, maybe I can add a new macro "ELF_LOAD_TOO_BIG" and
add the error message to load_elf_strerror().
I'll send a v2.
>
> I agree that accumulating the size in an 'int' is a bit
> dubious these days.
Maybe I can send another patch to change it and wherever it's used.
Thanks,
Stefano
Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
> ---
> 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?
Anyway:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> /* address_offset is hack for kernel images that are
> linked at the wrong physical address. */
> if (translate_fn) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 32f7cc7c33..feda52fa88 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -44,6 +44,7 @@
>
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> +#include "qemu/error-report.h"
> #include "qapi/error.h"
> #include "hw/hw.h"
> #include "disas/disas.h"
--
Alex Bennée
On Tue, Sep 10, 2019 at 10:50:30AM +0100, Alex Bennée wrote:
>
> Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
> > ---
> > 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 <alex.bennee@linaro.org>
>
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 :|
On Tue, Sep 10, 2019 at 10:50:30AM +0100, Alex Bennée wrote:
>
> Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
> > ---
> > 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 <alex.bennee@linaro.org>
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?
Thanks,
Stefano
Stefano Garzarella <sgarzare@redhat.com> writes:
> On Tue, Sep 10, 2019 at 10:50:30AM +0100, Alex Bennée wrote:
>>
>> Stefano Garzarella <sgarzare@redhat.com> 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 <sgarzare@redhat.com>
>> > ---
>> > 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 <alex.bennee@linaro.org>
>
> 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
On Tue, 10 Sep 2019 at 10:50, Alex Bennée <alex.bennee@linaro.org> wrote: > 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? Probably not, because one bit of cleanup we *should* be doing in the fail-exit codepaths but currently don't is to delete any rom blobs we created for earlier segments in the ELF file before we gave up, so we need to have an error-exit path anyway... thanks -- PMM
On Tue, Sep 10, 2019 at 10:54:25AM +0100, Peter Maydell wrote: > On Tue, 10 Sep 2019 at 10:50, Alex Bennée <alex.bennee@linaro.org> wrote: > > 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? > > Probably not, because one bit of cleanup we *should* be doing > in the fail-exit codepaths but currently don't is to delete > any rom blobs we created for earlier segments in the ELF file > before we gave up, so we need to have an error-exit path anyway... Mmm right, I should add a new API (e.g. rom_remove()) to do this better cleanup. > > thanks > -- PMM >
© 2016 - 2024 Red Hat, Inc.