[Qemu-devel] [PATCH 02/10] hw/ppc/ppc405_boards: Don't use load_image()

Peter Maydell posted 10 patches 7 years ago
[Qemu-devel] [PATCH 02/10] hw/ppc/ppc405_boards: Don't use load_image()
Posted by Peter Maydell 7 years ago
The load_image() function is deprecated, as it does not let the
caller specify how large the buffer to read the file into is.
Instead use load_image_size().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/ppc405_boards.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
index 3be3fe4432b..1b0a0a8ba3a 100644
--- a/hw/ppc/ppc405_boards.c
+++ b/hw/ppc/ppc405_boards.c
@@ -219,9 +219,11 @@ static void ref405ep_init(MachineState *machine)
             bios_name = BIOS_FILENAME;
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (filename) {
-            bios_size = load_image(filename, memory_region_get_ram_ptr(bios));
+            bios_size = load_image_size(filename,
+                                        memory_region_get_ram_ptr(bios),
+                                        BIOS_SIZE);
             g_free(filename);
-            if (bios_size < 0 || bios_size > BIOS_SIZE) {
+            if (bios_size < 0) {
                 error_report("Could not load PowerPC BIOS '%s'", bios_name);
                 exit(1);
             }
@@ -515,9 +517,11 @@ static void taihu_405ep_init(MachineState *machine)
                                &error_fatal);
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
         if (filename) {
-            bios_size = load_image(filename, memory_region_get_ram_ptr(bios));
+            bios_size = load_image_size(filename,
+                                        memory_region_get_ram_ptr(bios),
+                                        BIOS_SIZE);
             g_free(filename);
-            if (bios_size < 0 || bios_size > BIOS_SIZE) {
+            if (bios_size < 0) {
                 error_report("Could not load PowerPC BIOS '%s'", bios_name);
                 exit(1);
             }
-- 
2.19.1


Re: [Qemu-devel] [PATCH 02/10] hw/ppc/ppc405_boards: Don't use load_image()
Posted by Eric Blake 7 years ago
On 11/30/18 9:17 AM, Peter Maydell wrote:
> The load_image() function is deprecated, as it does not let the
> caller specify how large the buffer to read the file into is.
> Instead use load_image_size().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/ppc/ppc405_boards.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 3be3fe4432b..1b0a0a8ba3a 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -219,9 +219,11 @@ static void ref405ep_init(MachineState *machine)
>               bios_name = BIOS_FILENAME;
>           filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>           if (filename) {
> -            bios_size = load_image(filename, memory_region_get_ram_ptr(bios));
> +            bios_size = load_image_size(filename,
> +                                        memory_region_get_ram_ptr(bios),
> +                                        BIOS_SIZE);
>               g_free(filename);
> -            if (bios_size < 0 || bios_size > BIOS_SIZE) {

That old code is so wrong - "if we already overflowed the destination, 
possibly allowing for RCE in the meantime which might not even return to 
executing this code, THEN check and report the overflow".

> +            if (bios_size < 0) {
>                   error_report("Could not load PowerPC BIOS '%s'", bios_name);
>                   exit(1);
>               }

MUCH safer, even if silent truncation happens.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 02/10] hw/ppc/ppc405_boards: Don't use load_image()
Posted by David Gibson 7 years ago
On Fri, Nov 30, 2018 at 03:17:04PM +0000, Peter Maydell wrote:
> The load_image() function is deprecated, as it does not let the
> caller specify how large the buffer to read the file into is.
> Instead use load_image_size().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/ppc405_boards.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 3be3fe4432b..1b0a0a8ba3a 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -219,9 +219,11 @@ static void ref405ep_init(MachineState *machine)
>              bios_name = BIOS_FILENAME;
>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>          if (filename) {
> -            bios_size = load_image(filename, memory_region_get_ram_ptr(bios));
> +            bios_size = load_image_size(filename,
> +                                        memory_region_get_ram_ptr(bios),
> +                                        BIOS_SIZE);
>              g_free(filename);
> -            if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +            if (bios_size < 0) {
>                  error_report("Could not load PowerPC BIOS '%s'", bios_name);
>                  exit(1);
>              }
> @@ -515,9 +517,11 @@ static void taihu_405ep_init(MachineState *machine)
>                                 &error_fatal);
>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>          if (filename) {
> -            bios_size = load_image(filename, memory_region_get_ram_ptr(bios));
> +            bios_size = load_image_size(filename,
> +                                        memory_region_get_ram_ptr(bios),
> +                                        BIOS_SIZE);
>              g_free(filename);
> -            if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +            if (bios_size < 0) {
>                  error_report("Could not load PowerPC BIOS '%s'", bios_name);
>                  exit(1);
>              }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson