| Message ID | 20190910090821.28327-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | elf-ops.h: fix int overflow in load_elf() | expand |
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
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 >
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
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"
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(+)