hw/arm/nseries: Check return value from load_image_targphys()

[PATCH] hw/arm/nseries: Check return value from load_image_targphys()

Posted by Peter Maydell 3 years, 6 months ago

The nseries machines have a codepath that allows them to load a
secondary bootloader.  This code wasn't checking that the
load_image_targphys() succeeded.  Check the return value and report
the error to the user.

While we're in the vicinity, fix the comment style of the
comment documenting what this image load is doing.

Fixes: Coverity CID 1192904
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/nseries.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 76fd7fe9854..d49852c60d7 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -1380,7 +1380,8 @@ static void n8x0_init(MachineState *machine,
         /* No, wait, better start at the ROM.  */
         s->mpu->cpu->env.regs[15] = OMAP2_Q2_BASE + 0x400000;
 
-        /* This is intended for loading the `secondary.bin' program from
+        /*
+         * This is intended for loading the `secondary.bin' program from
          * Nokia images (the NOLO bootloader).  The entry point seems
          * to be at OMAP2_Q2_BASE + 0x400000.
          *
@@ -1388,9 +1389,15 @@ static void n8x0_init(MachineState *machine,
          * for them the entry point needs to be set to OMAP2_SRAM_BASE.
          *
          * The code above is for loading the `zImage' file from Nokia
-         * images.  */
-        load_image_targphys(option_rom[0].name, OMAP2_Q2_BASE + 0x400000,
-                            machine->ram_size - 0x400000);
+         * images.
+         */
+        if (load_image_targphys(option_rom[0].name,
+                                OMAP2_Q2_BASE + 0x400000,
+                                machine->ram_size - 0x400000) < 0) {
+            error_report("Failed to load secondary bootloader %s",
+                         option_rom[0].name);
+            exit(EXIT_FAILURE);
+        }
 
         n800_setup_nolo_tags(nolo_tags);
         cpu_physical_memory_write(OMAP2_SRAM_BASE, nolo_tags, 0x10000);
-- 
2.20.1

Re: [PATCH] hw/arm/nseries: Check return value from load_image_targphys()

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 11/3/20 12:49 PM, Peter Maydell wrote:
> The nseries machines have a codepath that allows them to load a
> secondary bootloader.  This code wasn't checking that the
> load_image_targphys() succeeded.  Check the return value and report
> the error to the user.
> 
> While we're in the vicinity, fix the comment style of the
> comment documenting what this image load is doing.
> 
> Fixes: Coverity CID 1192904
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/nseries.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
> index 76fd7fe9854..d49852c60d7 100644
> --- a/hw/arm/nseries.c
> +++ b/hw/arm/nseries.c
> @@ -1380,7 +1380,8 @@ static void n8x0_init(MachineState *machine,
>          /* No, wait, better start at the ROM.  */
>          s->mpu->cpu->env.regs[15] = OMAP2_Q2_BASE + 0x400000;
>  
> -        /* This is intended for loading the `secondary.bin' program from
> +        /*
> +         * This is intended for loading the `secondary.bin' program from
>           * Nokia images (the NOLO bootloader).  The entry point seems
>           * to be at OMAP2_Q2_BASE + 0x400000.
>           *
> @@ -1388,9 +1389,15 @@ static void n8x0_init(MachineState *machine,
>           * for them the entry point needs to be set to OMAP2_SRAM_BASE.
>           *
>           * The code above is for loading the `zImage' file from Nokia
> -         * images.  */
> -        load_image_targphys(option_rom[0].name, OMAP2_Q2_BASE + 0x400000,
> -                            machine->ram_size - 0x400000);
> +         * images.
> +         */
> +        if (load_image_targphys(option_rom[0].name,
> +                                OMAP2_Q2_BASE + 0x400000,
> +                                machine->ram_size - 0x400000) < 0) {
> +            error_report("Failed to load secondary bootloader %s",
> +                         option_rom[0].name);
> +            exit(EXIT_FAILURE);
> +        }
>  
>          n800_setup_nolo_tags(nolo_tags);
>          cpu_physical_memory_write(OMAP2_SRAM_BASE, nolo_tags, 0x10000);
> 

What about the other cases?

$ git grep -E ^\\s+load_image_targphys
hw/alpha/dp264.c:163:            load_image_targphys(initrd_filename,
initrd_base,
hw/hppa/machine.c:301:            load_image_targphys(initrd_filename,
initrd_base, initrd_size);
hw/m68k/q800.c:388:            load_image_targphys(initrd_filename,
initrd_base,

Thanks,

Phil.

Re: [PATCH] hw/arm/nseries: Check return value from load_image_targphys()

Posted by Peter Maydell 3 years, 6 months ago

On Tue, 3 Nov 2020 at 21:00, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 11/3/20 12:49 PM, Peter Maydell wrote:
> > The nseries machines have a codepath that allows them to load a
> > secondary bootloader.  This code wasn't checking that the
> > load_image_targphys() succeeded.  Check the return value and report
> > the error to the user.
> >
> > While we're in the vicinity, fix the comment style of the
> > comment documenting what this image load is doing.
> >
> > Fixes: Coverity CID 1192904
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> What about the other cases?
>
> $ git grep -E ^\\s+load_image_targphys
> hw/alpha/dp264.c:163:            load_image_targphys(initrd_filename,
> initrd_base,
> hw/hppa/machine.c:301:            load_image_targphys(initrd_filename,
> initrd_base, initrd_size);
> hw/m68k/q800.c:388:            load_image_targphys(initrd_filename,
> initrd_base,

They could be fixed too, certainly; I only care about the ones
Coverity is currently complaining about, though :-)

-- PMM