* Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
2020-04-23 20:20 [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL) Peter Maydell
@ 2020-04-23 20:24 ` Philippe Mathieu-Daudé
2020-04-23 20:47 ` Peter Maydell
2020-04-24 8:02 ` Stefano Garzarella
2020-05-04 9:46 ` Laurent Vivier
2 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-23 20:24 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial; +Cc: Randy Yates
On 4/23/20 10:20 PM, Peter Maydell wrote:
> Calling g_mapped_file_unref() on a NULL pointer is not valid, and
> glib will assert if you try it.
>
> $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf
> qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: assertion 'file != NULL' failed
>
> (One way to produce an ELF file that fails like this is to copy just
> the first 16 bytes of a valid ELF file; this is sufficient to fool
> the code in load_elf_ram_sym() into thinking it's an ELF file and
> calling load_elf32() or load_elf64().)
>
> The failure-exit path in load_elf can be reached from various points
> in execution, and for some of those we haven't yet called
> g_mapped_file_new_from_fd(). Add a condition to the unref call so we
> only call it if we successfully created the GMappedFile to start with.
>
> This will fix the assertion; for the specific case of the generic
> loader it will then fall back from "guess this is an ELF file" to
> "maybe it's a uImage or a hex file" and eventually to "just load as
> a raw data file".
>
> Reported-by: Randy Yates <yates@ieee.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> include/hw/elf_ops.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index e0bb47bb678..398a4a2c85b 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> *highaddr = (uint64_t)(elf_sword)high;
> ret = total_size;
> fail:
> - g_mapped_file_unref(mapped_file);
> + if (mapped_file) {
> + g_mapped_file_unref(mapped_file);
> + }
> g_free(phdr);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
2020-04-23 20:24 ` Philippe Mathieu-Daudé
@ 2020-04-23 20:47 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2020-04-23 20:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: QEMU Trivial, Randy Yates, QEMU Developers
On Thu, 23 Apr 2020 at 21:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 4/23/20 10:20 PM, Peter Maydell wrote:
> > This will fix the assertion; for the specific case of the generic
> > loader it will then fall back from "guess this is an ELF file" to
> > "maybe it's a uImage or a hex file" and eventually to "just load as
> > a raw data file".
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thanks; as a side note "let me just load this as a raw data file"
is not a very user-friendly way to report issues with the ELF
file like "seems to be truncated" or "has no program headers".
Not sure what to do about that...
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
2020-04-23 20:20 [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL) Peter Maydell
2020-04-23 20:24 ` Philippe Mathieu-Daudé
@ 2020-04-24 8:02 ` Stefano Garzarella
2020-05-04 9:46 ` Laurent Vivier
2 siblings, 0 replies; 5+ messages in thread
From: Stefano Garzarella @ 2020-04-24 8:02 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-trivial, Randy Yates, qemu-devel
On Thu, Apr 23, 2020 at 09:20:11PM +0100, Peter Maydell wrote:
> Calling g_mapped_file_unref() on a NULL pointer is not valid, and
> glib will assert if you try it.
>
> $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf
> qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: assertion 'file != NULL' failed
>
> (One way to produce an ELF file that fails like this is to copy just
> the first 16 bytes of a valid ELF file; this is sufficient to fool
> the code in load_elf_ram_sym() into thinking it's an ELF file and
> calling load_elf32() or load_elf64().)
>
> The failure-exit path in load_elf can be reached from various points
> in execution, and for some of those we haven't yet called
> g_mapped_file_new_from_fd(). Add a condition to the unref call so we
> only call it if we successfully created the GMappedFile to start with.
>
> This will fix the assertion; for the specific case of the generic
> loader it will then fall back from "guess this is an ELF file" to
> "maybe it's a uImage or a hex file" and eventually to "just load as
> a raw data file".
>
> Reported-by: Randy Yates <yates@ieee.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/elf_ops.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index e0bb47bb678..398a4a2c85b 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> *highaddr = (uint64_t)(elf_sword)high;
> ret = total_size;
> fail:
> - g_mapped_file_unref(mapped_file);
> + if (mapped_file) {
> + g_mapped_file_unref(mapped_file);
> + }
> g_free(phdr);
> return ret;
> }
Oooops, my fault :-(
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Maybe we can add:
Fixes: 816b9fe450 ("elf-ops.h: Map into memory the ELF to load")
Thanks,
Stefano
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL)
2020-04-23 20:20 [PATCH] elf_ops: Don't try to g_mapped_file_unref(NULL) Peter Maydell
2020-04-23 20:24 ` Philippe Mathieu-Daudé
2020-04-24 8:02 ` Stefano Garzarella
@ 2020-05-04 9:46 ` Laurent Vivier
2 siblings, 0 replies; 5+ messages in thread
From: Laurent Vivier @ 2020-05-04 9:46 UTC (permalink / raw)
To: Peter Maydell, qemu-devel, qemu-trivial; +Cc: Randy Yates
Le 23/04/2020 à 22:20, Peter Maydell a écrit :
> Calling g_mapped_file_unref() on a NULL pointer is not valid, and
> glib will assert if you try it.
>
> $ qemu-system-arm -M virt -display none -device loader,file=/tmp/bad.elf
> qemu-system-arm: -device loader,file=/tmp/bad.elf: GLib: g_mapped_file_unref: assertion 'file != NULL' failed
>
> (One way to produce an ELF file that fails like this is to copy just
> the first 16 bytes of a valid ELF file; this is sufficient to fool
> the code in load_elf_ram_sym() into thinking it's an ELF file and
> calling load_elf32() or load_elf64().)
>
> The failure-exit path in load_elf can be reached from various points
> in execution, and for some of those we haven't yet called
> g_mapped_file_new_from_fd(). Add a condition to the unref call so we
> only call it if we successfully created the GMappedFile to start with.
>
> This will fix the assertion; for the specific case of the generic
> loader it will then fall back from "guess this is an ELF file" to
> "maybe it's a uImage or a hex file" and eventually to "just load as
> a raw data file".
>
> Reported-by: Randy Yates <yates@ieee.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/elf_ops.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index e0bb47bb678..398a4a2c85b 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -606,7 +606,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> *highaddr = (uint64_t)(elf_sword)high;
> ret = total_size;
> fail:
> - g_mapped_file_unref(mapped_file);
> + if (mapped_file) {
> + g_mapped_file_unref(mapped_file);
> + }
> g_free(phdr);
> return ret;
> }
>
Applied to my trivial-patches branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 5+ messages in thread