diff mbox series

[3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument

Message ID 20200309144353.26457-4-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/core/loader: Prohibit loading ROMs bigger than memory region | expand

Commit Message

Philippe Mathieu-Daudé March 9, 2020, 2:43 p.m. UTC 
In commit d60fa42e8b we cared about loading ROMs smaller than
the underlying memory region (by zeroing the missing area) but
we forgot the case where a ROM file is bigger than the underlying
region.
Provide rom_add_file() a 'max_size' argument, and refuse loading
ROM files bigger than it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/loader.h | 12 ++++++------
 hw/core/loader.c    | 19 +++++++++++++------
 2 files changed, 19 insertions(+), 12 deletions(-)

Comments

Taylor Simpson March 9, 2020, 5:44 p.m. UTC | #1 
> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+tsimpson=quicinc.com@nongnu.org> On Behalf Of Philippe
> Mathieu-Daudé
> Sent: Monday, March 9, 2020 9:44 AM
> To: qemu-devel@nongnu.org
> Cc: Eduardo Habkost <ehabkost@redhat.com>; Michael S. Tsirkin
> <mst@redhat.com>; Alistair Francis <alistair@alistair23.me>; Mark Cave-
> Ayland <mark.cave-ayland@ilande.co.uk>; qemu-ppc@nongnu.org; Gerd
> Hoffmann <kraxel@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>;
> David Gibson <david@gibson.dropbear.id.au>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Artyom Tarasenko <atar4qemu@gmail.com>;
> Richard Henderson <rth@twiddle.net>
> Subject: [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size'
> argument
>
>
> -#define rom_add_file_fixed(_f, _a, _i)          \
> -    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
> +#define rom_add_file_fixed(_f, _a, _i) \
> +    rom_add_file(_f, NULL, _a, -1, _i, false, NULL, NULL)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>      rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
>  #define rom_add_file_mr(_f, _mr, _i)            \
> -    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
> +    rom_add_file(_f, NULL, 0, -1, _i, false, _mr, NULL)
>  #define rom_add_file_as(_f, _as, _i)            \
> -    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
> +    rom_add_file(_f, NULL, 0, -1, _i, false, NULL, _as)
>  #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
> -    rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
> +    rom_add_file(_f, NULL, _a, -1, _i, false, NULL, _as)
>  #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
>      rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)

The above looks like -1 represents an unknown max_len.

> +    rom->romsize = max_len ? max_len : rom->datasize;

So, shouldn't this check max_len == -1?  In general, wouldn't it be better to #define UNKNOWN_MAX_LEN to make it more clear what the value is used for?
diff mbox series

Patch

diff --git a/include/hw/loader.h b/include/hw/loader.h
index a9eeea3952..99d690a628 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -254,7 +254,7 @@  extern bool option_rom_has_mr;
 extern bool rom_file_has_mr;
 
 int rom_add_file(const char *file, const char *fw_dir,
-                 hwaddr addr, int32_t bootindex,
+                 hwaddr addr, uint64_t max_len, int32_t bootindex,
                  bool option_rom, MemoryRegion *mr, AddressSpace *as);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
@@ -292,16 +292,16 @@  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
 void *rom_ptr(hwaddr addr, size_t size);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
-#define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
+#define rom_add_file_fixed(_f, _a, _i) \
+    rom_add_file(_f, NULL, _a, -1, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
 #define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+    rom_add_file(_f, NULL, 0, -1, _i, false, _mr, NULL)
 #define rom_add_file_as(_f, _as, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
+    rom_add_file(_f, NULL, 0, -1, _i, false, NULL, _as)
 #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
+    rom_add_file(_f, NULL, _a, -1, _i, false, NULL, _as)
 #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index c67c483936..00436f1524 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -938,7 +938,7 @@  static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
 }
 
 int rom_add_file(const char *file, const char *fw_dir,
-                 hwaddr addr, int32_t bootindex,
+                 hwaddr addr, uint64_t max_len, int32_t bootindex,
                  bool option_rom, MemoryRegion *mr,
                  AddressSpace *as)
 {
@@ -974,14 +974,21 @@  int rom_add_file(const char *file, const char *fw_dir,
         rom->fw_file = g_strdup(file);
     }
     rom->addr     = addr;
-    rom->romsize  = lseek(fd, 0, SEEK_END);
-    if (rom->romsize == -1) {
+    rom->datasize = lseek(fd, 0, SEEK_END);
+    if (rom->datasize == -1) {
         fprintf(stderr, "rom: file %-20s: get size error: %s\n",
                 rom->name, strerror(errno));
         goto err;
     }
 
-    rom->datasize = rom->romsize;
+    rom->romsize = max_len ? max_len : rom->datasize;
+    if (rom->romsize < rom->datasize) {
+        fprintf(stderr,
+                "rom: file '%-20s' size too big: %zu (available: %zu)\n",
+                rom->name, rom->datasize, rom->romsize);
+        goto err;
+    }
+
     rom->data     = g_malloc0(rom->datasize);
     lseek(fd, 0, SEEK_SET);
     rc = read(fd, rom->data, rom->datasize);
@@ -1107,12 +1114,12 @@  int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
 
 int rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, -1, true, NULL, NULL);
 }
 
 int rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
+    return rom_add_file(file, "genroms", 0, -1, bootindex, true, NULL, NULL);
 }
 
 static void rom_reset(void *unused)