[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] elf-ops.h: fix int overflow in load_elf()
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH] elf-ops.h: fix int overflow in load_elf() |
Date: |
Tue, 10 Sep 2019 10:50:30 +0100 |
User-agent: |
mu4e 1.3.4; emacs 27.0.50 |
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?
Anyway:
Reviewed-by: Alex Bennée <address@hidden>
> /* 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