All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region
@ 2020-03-09 14:43 Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 1/5] hw/sparc64/niagara: Pass available memory region size to add_rom_or_fail Philippe Mathieu-Daudé
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

This series fixes a bug where an user can load ROMs bigger
than the expected size. The bug is delayed after a reset where
rom_reset() can overflow the underlying memory regions.

Philippe Mathieu-Daudé (5):
  hw/sparc64/niagara: Pass available memory region size to
    add_rom_or_fail
  hw/core/loader: Prohibit loading ROMs bigger than memory region
  hw/core/loader: Provide rom_add_file() a 'max_size' argument
  hw/core/loader: Restrict rom_add_file_mr() to available region size
  hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument

 include/hw/loader.h  | 12 ++++++------
 hw/core/loader.c     | 24 +++++++++++++++++-------
 hw/i386/x86.c        |  2 +-
 hw/ppc/sam460ex.c    |  2 +-
 hw/sparc64/niagara.c | 22 +++++++++++++---------
 5 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.21.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/5] hw/sparc64/niagara: Pass available memory region size to add_rom_or_fail
  2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
@ 2020-03-09 14:43 ` Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

In few commits we'll let rom_add_file_fixed() take a 'size'
argument holding the maximum file length that can be loaded
as ROM. In preparation, modify the Niagara machine, so the
generic change will be easier to review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sparc64/niagara.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index ab5ef8c5b3..f58d008d3d 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -85,7 +85,8 @@ typedef struct NiagaraBoardState {
 #define NIAGARA_OBP_OFFSET  0x80000ULL
 #define PROM_SIZE_MAX       (4 * MiB)
 
-static void add_rom_or_fail(const char *file, const hwaddr addr)
+static void add_rom_or_fail(const char *file, const hwaddr addr,
+                            uint64_t region_size)
 {
     /* XXX remove qtest_enabled() check once firmware files are
      * in the qemu tree
@@ -126,13 +127,15 @@ static void niagara_init(MachineState *machine)
                            &error_fatal);
     memory_region_add_subregion(sysmem, NIAGARA_PROM_BASE, &s->prom);
 
-    add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE);
-    add_rom_or_fail("1up-md.bin", NIAGARA_MD_ROM_BASE);
-    add_rom_or_fail("1up-hv.bin", NIAGARA_HV_ROM_BASE);
+    add_rom_or_fail("nvram1", NIAGARA_NVRAM_BASE, NIAGARA_NVRAM_SIZE);
+    add_rom_or_fail("1up-md.bin", NIAGARA_MD_ROM_BASE, NIAGARA_MD_ROM_SIZE);
+    add_rom_or_fail("1up-hv.bin", NIAGARA_HV_ROM_BASE, NIAGARA_HV_ROM_SIZE);
 
-    add_rom_or_fail("reset.bin", NIAGARA_PROM_BASE);
-    add_rom_or_fail("q.bin", NIAGARA_PROM_BASE + NIAGARA_Q_OFFSET);
-    add_rom_or_fail("openboot.bin", NIAGARA_PROM_BASE + NIAGARA_OBP_OFFSET);
+    add_rom_or_fail("reset.bin", NIAGARA_PROM_BASE, NIAGARA_Q_OFFSET);
+    add_rom_or_fail("q.bin", NIAGARA_PROM_BASE + NIAGARA_Q_OFFSET,
+                    NIAGARA_OBP_OFFSET);
+    add_rom_or_fail("openboot.bin", NIAGARA_PROM_BASE + NIAGARA_OBP_OFFSET,
+                    PROM_SIZE_MAX - NIAGARA_OBP_OFFSET);
 
     /* the virtual ramdisk is kind of initrd, but it resides
        outside of the partition RAM */
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region
  2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 1/5] hw/sparc64/niagara: Pass available memory region size to add_rom_or_fail Philippe Mathieu-Daudé
@ 2020-03-09 14:43 ` Philippe Mathieu-Daudé
  2020-03-09 14:48   ` Peter Maydell
  2020-03-09 14:43 ` [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

We must not write more data than the memory region size.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/loader.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index d1b78f60cd..c67c483936 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1136,7 +1136,10 @@ static void rom_reset(void *unused)
             continue;
         }
         if (rom->mr) {
-            void *host = memory_region_get_ram_ptr(rom->mr);
+            void *host;
+
+            assert(memory_region_size(rom->mr) >= rom->datasize);
+            host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument
  2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 1/5] hw/sparc64/niagara: Pass available memory region size to add_rom_or_fail Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
@ 2020-03-09 14:43 ` Philippe Mathieu-Daudé
  2020-03-09 17:44   ` Taylor Simpson
  2020-03-09 14:43 ` [PATCH 4/5] hw/core/loader: Restrict rom_add_file_mr() to available region size Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument Philippe Mathieu-Daudé
  4 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

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(-)

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)
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/5] hw/core/loader: Restrict rom_add_file_mr() to available region size
  2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-03-09 14:43 ` [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument Philippe Mathieu-Daudé
@ 2020-03-09 14:43 ` Philippe Mathieu-Daudé
  2020-03-09 14:43 ` [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument Philippe Mathieu-Daudé
  4 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

Use the ''max_size' argument recently added to rom_add_file() to
not load ROMs bigger than the underlying memory region.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/loader.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 99d690a628..34ac630eb1 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -297,7 +297,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
 #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, -1, _i, false, _mr, NULL)
+    rom_add_file(_f, NULL, 0, memory_region_size(_mr), _i, false, _mr, NULL)
 #define rom_add_file_as(_f, _as, _i)            \
     rom_add_file(_f, NULL, 0, -1, _i, false, NULL, _as)
 #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument
  2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2020-03-09 14:43 ` [PATCH 4/5] hw/core/loader: Restrict rom_add_file_mr() to available region size Philippe Mathieu-Daudé
@ 2020-03-09 14:43 ` Philippe Mathieu-Daudé
  2020-03-10  0:43   ` David Gibson
  4 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 14:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	David Gibson, Philippe Mathieu-Daudé,
	Artyom Tarasenko, Richard Henderson

Let rom_add_file_fixed() call rom_add_file() with a 'max_size'
argument, to avoid writing more than the available space for
the ROMs.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/loader.h  | 4 ++--
 hw/i386/x86.c        | 2 +-
 hw/ppc/sam460ex.c    | 2 +-
 hw/sparc64/niagara.c | 5 +++--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index 34ac630eb1..30ed80128e 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -292,8 +292,8 @@ 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, -1, _i, false, NULL, NULL)
+#define rom_add_file_fixed(_f, _a, _ms, _i) \
+    rom_add_file(_f, NULL, _a, _ms, _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)            \
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 7f38e6ba8b..bdac66206a 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -783,7 +783,7 @@ void x86_bios_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
     if (!isapc_ram_fw) {
         memory_region_set_readonly(bios, true);
     }
-    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
+    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), bios_size, -1);
     if (ret != 0) {
     bios_error:
         fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 898453cf30..5eab479ae5 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -127,7 +127,7 @@ static int sam460ex_load_uboot(void)
                 " using default u-boot image");*/
         rom_add_file_fixed(UBOOT_FILENAME,
                            UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32),
-                           -1);
+                           UBOOT_SIZE, -1);
     }
 
     return 0;
diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index f58d008d3d..4dd9a77dcb 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -91,7 +91,7 @@ static void add_rom_or_fail(const char *file, const hwaddr addr,
     /* XXX remove qtest_enabled() check once firmware files are
      * in the qemu tree
      */
-    if (!qtest_enabled() && rom_add_file_fixed(file, addr, -1)) {
+    if (!qtest_enabled() && rom_add_file_fixed(file, addr, region_size, -1)) {
         error_report("Unable to load a firmware for -M niagara");
         exit(1);
     }
@@ -148,7 +148,8 @@ static void niagara_init(MachineState *machine)
             memory_region_add_subregion(get_system_memory(),
                                         NIAGARA_VDISK_BASE, &s->vdisk_ram);
             dinfo->is_default = 1;
-            rom_add_file_fixed(blk_bs(blk)->filename, NIAGARA_VDISK_BASE, -1);
+            rom_add_file_fixed(blk_bs(blk)->filename, NIAGARA_VDISK_BASE,
+                               size, -1);
         } else {
             error_report("could not load ram disk '%s'",
                          blk_bs(blk)->filename);
-- 
2.21.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region
  2020-03-09 14:43 ` [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
@ 2020-03-09 14:48   ` Peter Maydell
  2020-03-09 15:41     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2020-03-09 14:48 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, QEMU Developers, qemu-ppc, Gerd Hoffmann,
	Paolo Bonzini, Richard Henderson, Artyom Tarasenko, David Gibson

On Mon, 9 Mar 2020 at 14:45, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> We must not write more data than the memory region size.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/loader.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index d1b78f60cd..c67c483936 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1136,7 +1136,10 @@ static void rom_reset(void *unused)
>              continue;
>          }
>          if (rom->mr) {
> -            void *host = memory_region_get_ram_ptr(rom->mr);
> +            void *host;
> +
> +            assert(memory_region_size(rom->mr) >= rom->datasize);
> +            host = memory_region_get_ram_ptr(rom->mr);
>              memcpy(host, rom->data, rom->datasize);

Does this really only happen if there's a QEMU bug,
or could a user trigger this assert by accidentally
passing an oversize file on the command line?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region
  2020-03-09 14:48   ` Peter Maydell
@ 2020-03-09 15:41     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-09 15:41 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, QEMU Developers, qemu-ppc, Gerd Hoffmann,
	Paolo Bonzini, Richard Henderson, Artyom Tarasenko, David Gibson

On 3/9/20 3:48 PM, Peter Maydell wrote:
> On Mon, 9 Mar 2020 at 14:45, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> We must not write more data than the memory region size.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/core/loader.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index d1b78f60cd..c67c483936 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -1136,7 +1136,10 @@ static void rom_reset(void *unused)
>>               continue;
>>           }
>>           if (rom->mr) {
>> -            void *host = memory_region_get_ram_ptr(rom->mr);
>> +            void *host;
>> +
>> +            assert(memory_region_size(rom->mr) >= rom->datasize);
>> +            host = memory_region_get_ram_ptr(rom->mr);
>>               memcpy(host, rom->data, rom->datasize);
> 
> Does this really only happen if there's a QEMU bug,
> or could a user trigger this assert by accidentally
> passing an oversize file on the command line?

This is definitively command-line triggered, but it occurred when I was 
trying the RX port (not yet merged) so the bug might be an incorrect API 
use there. I'll check tonight. Meanwhile I went consolidating the rest 
of the code to feel safer.

> 
> thanks
> -- PMM
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument
  2020-03-09 14:43 ` [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument Philippe Mathieu-Daudé
@ 2020-03-09 17:44   ` Taylor Simpson
  0 siblings, 0 replies; 10+ messages in thread
From: Taylor Simpson @ 2020-03-09 17:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-ppc, Gerd Hoffmann, Paolo Bonzini,
	Richard Henderson, Artyom Tarasenko, David Gibson



> -----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?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument
  2020-03-09 14:43 ` [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument Philippe Mathieu-Daudé
@ 2020-03-10  0:43   ` David Gibson
  0 siblings, 0 replies; 10+ messages in thread
From: David Gibson @ 2020-03-10  0:43 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Eduardo Habkost, Michael S. Tsirkin, Alistair Francis,
	Mark Cave-Ayland, qemu-devel, qemu-ppc, Gerd Hoffmann,
	Paolo Bonzini, Artyom Tarasenko, Richard Henderson

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

On Mon, Mar 09, 2020 at 03:43:53PM +0100, Philippe Mathieu-Daudé wrote:
> Let rom_add_file_fixed() call rom_add_file() with a 'max_size'
> argument, to avoid writing more than the available space for
> the ROMs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

ppc parts

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

> ---
>  include/hw/loader.h  | 4 ++--
>  hw/i386/x86.c        | 2 +-
>  hw/ppc/sam460ex.c    | 2 +-
>  hw/sparc64/niagara.c | 5 +++--
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 34ac630eb1..30ed80128e 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -292,8 +292,8 @@ 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, -1, _i, false, NULL, NULL)
> +#define rom_add_file_fixed(_f, _a, _ms, _i) \
> +    rom_add_file(_f, NULL, _a, _ms, _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)            \
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 7f38e6ba8b..bdac66206a 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -783,7 +783,7 @@ void x86_bios_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>      if (!isapc_ram_fw) {
>          memory_region_set_readonly(bios, true);
>      }
> -    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
> +    ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), bios_size, -1);
>      if (ret != 0) {
>      bios_error:
>          fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 898453cf30..5eab479ae5 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -127,7 +127,7 @@ static int sam460ex_load_uboot(void)
>                  " using default u-boot image");*/
>          rom_add_file_fixed(UBOOT_FILENAME,
>                             UBOOT_LOAD_BASE | ((hwaddr)FLASH_BASE_H << 32),
> -                           -1);
> +                           UBOOT_SIZE, -1);
>      }
>  
>      return 0;
> diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> index f58d008d3d..4dd9a77dcb 100644
> --- a/hw/sparc64/niagara.c
> +++ b/hw/sparc64/niagara.c
> @@ -91,7 +91,7 @@ static void add_rom_or_fail(const char *file, const hwaddr addr,
>      /* XXX remove qtest_enabled() check once firmware files are
>       * in the qemu tree
>       */
> -    if (!qtest_enabled() && rom_add_file_fixed(file, addr, -1)) {
> +    if (!qtest_enabled() && rom_add_file_fixed(file, addr, region_size, -1)) {
>          error_report("Unable to load a firmware for -M niagara");
>          exit(1);
>      }
> @@ -148,7 +148,8 @@ static void niagara_init(MachineState *machine)
>              memory_region_add_subregion(get_system_memory(),
>                                          NIAGARA_VDISK_BASE, &s->vdisk_ram);
>              dinfo->is_default = 1;
> -            rom_add_file_fixed(blk_bs(blk)->filename, NIAGARA_VDISK_BASE, -1);
> +            rom_add_file_fixed(blk_bs(blk)->filename, NIAGARA_VDISK_BASE,
> +                               size, -1);
>          } else {
>              error_report("could not load ram disk '%s'",
>                           blk_bs(blk)->filename);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-03-10  0:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-09 14:43 [PATCH 0/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
2020-03-09 14:43 ` [PATCH 1/5] hw/sparc64/niagara: Pass available memory region size to add_rom_or_fail Philippe Mathieu-Daudé
2020-03-09 14:43 ` [PATCH 2/5] hw/core/loader: Prohibit loading ROMs bigger than memory region Philippe Mathieu-Daudé
2020-03-09 14:48   ` Peter Maydell
2020-03-09 15:41     ` Philippe Mathieu-Daudé
2020-03-09 14:43 ` [PATCH 3/5] hw/core/loader: Provide rom_add_file() a 'max_size' argument Philippe Mathieu-Daudé
2020-03-09 17:44   ` Taylor Simpson
2020-03-09 14:43 ` [PATCH 4/5] hw/core/loader: Restrict rom_add_file_mr() to available region size Philippe Mathieu-Daudé
2020-03-09 14:43 ` [PATCH 5/5] hw/core/loader: Provide rom_add_file_fixed() a 'max_size' argument Philippe Mathieu-Daudé
2020-03-10  0:43   ` David Gibson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.