* [PATCH 0/2] Fix integer overflows in loading of large images
@ 2021-11-11 14:11 Jamie Iles
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
2021-11-11 14:11 ` [PATCH 2/2] hw/core/loader: workaround read() size limit Jamie Iles
0 siblings, 2 replies; 13+ messages in thread
From: Jamie Iles @ 2021-11-11 14:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Jamie Iles, lmichel
Most of the loader code currently uses a ssize_t or 64 bit integer type
to store image lengths, but many functions that handle loading return an
int with a negative value on error or length on success. Once an image
exceeds 2GB this will cause an integer overflow and so can end up
loading truncated images, silently failing to load an image (a 4GB image
would be interpreted as 0 bytes long).
This is unlikely to affect many deployments, but can manifest when
preloading RAM disks for example.
This builds upon 8975eb891fb6 ("hw/elf_ops.h: switch to ssize_t for elf
loader return type") to cover more of the generic loader.
Jamie Iles (2):
hw/core/loader: return image sizes as ssize_t
hw/core/loader: workaround read() size limit.
hw/arm/armv7m.c | 2 +-
hw/arm/boot.c | 8 +--
hw/core/generic-loader.c | 2 +-
hw/core/loader.c | 121 ++++++++++++++++++++++++---------------
hw/i386/x86.c | 2 +-
hw/riscv/boot.c | 5 +-
include/hw/loader.h | 55 +++++++++---------
7 files changed, 114 insertions(+), 81 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
2021-11-11 14:11 [PATCH 0/2] Fix integer overflows in loading of large images Jamie Iles
@ 2021-11-11 14:11 ` Jamie Iles
2021-11-11 14:20 ` Philippe Mathieu-Daudé
` (3 more replies)
2021-11-11 14:11 ` [PATCH 2/2] hw/core/loader: workaround read() size limit Jamie Iles
1 sibling, 4 replies; 13+ messages in thread
From: Jamie Iles @ 2021-11-11 14:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Jamie Iles, lmichel
Various loader functions return an int which limits images to 2GB which
is fine for things like a BIOS/kernel image, but if we want to be able
to load memory images or large ramdisks then any file over 2GB would
silently fail to load.
Cc: Luc Michel <lmichel@kalray.eu>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
hw/arm/armv7m.c | 2 +-
hw/arm/boot.c | 8 ++--
hw/core/generic-loader.c | 2 +-
hw/core/loader.c | 81 +++++++++++++++++++++-------------------
hw/i386/x86.c | 2 +-
hw/riscv/boot.c | 5 ++-
include/hw/loader.h | 55 +++++++++++++--------------
7 files changed, 80 insertions(+), 75 deletions(-)
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 8d08db80be83..a6393dce7276 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
{
- int image_size;
+ ssize_t image_size;
uint64_t entry;
int big_endian;
AddressSpace *as;
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 74ad397b1ff9..3853203438ba 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
return 0;
}
-static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
+static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
uint64_t *lowaddr, uint64_t *highaddr,
int elf_machine, AddressSpace *as)
{
@@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
} elf_header;
int data_swab = 0;
bool big_endian;
- int64_t ret = -1;
+ ssize_t ret = -1;
Error *err = NULL;
@@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
/* Set up for a direct boot of a kernel image file. */
CPUState *cs;
AddressSpace *as = arm_boot_address_space(cpu, info);
- int kernel_size;
+ ssize_t kernel_size;
int initrd_size;
int is_linux = 0;
uint64_t elf_entry;
@@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
if (kernel_size > info->ram_size) {
error_report("kernel '%s' is too large to fit in RAM "
- "(kernel size %d, RAM size %" PRId64 ")",
+ "(kernel size %zd, RAM size %" PRId64 ")",
info->kernel_filename, kernel_size, info->ram_size);
exit(1);
}
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index d14f932eea2e..bc1451da8f55 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
GenericLoaderState *s = GENERIC_LOADER(dev);
hwaddr entry;
int big_endian;
- int size = 0;
+ ssize_t size = 0;
s->set_pc = false;
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 052a0fd7198b..348bbf535bd9 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
return did;
}
-int load_image_targphys(const char *filename,
- hwaddr addr, uint64_t max_sz)
+ssize_t load_image_targphys(const char *filename,
+ hwaddr addr, uint64_t max_sz)
{
return load_image_targphys_as(filename, addr, max_sz, NULL);
}
/* return the size or -1 if error */
-int load_image_targphys_as(const char *filename,
- hwaddr addr, uint64_t max_sz, AddressSpace *as)
+ssize_t load_image_targphys_as(const char *filename,
+ hwaddr addr, uint64_t max_sz, AddressSpace *as)
{
- int size;
+ ssize_t size;
size = get_image_size(filename);
if (size < 0 || size > max_sz) {
@@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
return size;
}
-int load_image_mr(const char *filename, MemoryRegion *mr)
+ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
{
- int size;
+ ssize_t size;
if (!memory_access_is_direct(mr, false)) {
/* Can only load an image into RAM or ROM */
@@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
: (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), target_page_size)))
-int load_aout(const char *filename, hwaddr addr, int max_sz,
- int bswap_needed, hwaddr target_page_size)
+ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
+ int bswap_needed, hwaddr target_page_size)
{
int fd;
ssize_t size, ret;
@@ -618,13 +618,14 @@ toosmall:
}
/* Load a U-Boot image. */
-static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
- int *is_linux, uint8_t image_type,
- uint64_t (*translate_fn)(void *, uint64_t),
- void *translate_opaque, AddressSpace *as)
+static ssize_t load_uboot_image(const char *filename, hwaddr *ep,
+ hwaddr *loadaddr, int *is_linux,
+ uint8_t image_type,
+ uint64_t (*translate_fn)(void *, uint64_t),
+ void *translate_opaque, AddressSpace *as)
{
int fd;
- int size;
+ ssize_t size;
hwaddr address;
uboot_image_header_t h;
uboot_image_header_t *hdr = &h;
@@ -746,40 +747,40 @@ out:
return ret;
}
-int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
- int *is_linux,
- uint64_t (*translate_fn)(void *, uint64_t),
- void *translate_opaque)
+ssize_t load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
+ int *is_linux,
+ uint64_t (*translate_fn)(void *, uint64_t),
+ void *translate_opaque)
{
return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
translate_fn, translate_opaque, NULL);
}
-int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
- int *is_linux,
- uint64_t (*translate_fn)(void *, uint64_t),
- void *translate_opaque, AddressSpace *as)
+ssize_t load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
+ int *is_linux,
+ uint64_t (*translate_fn)(void *, uint64_t),
+ void *translate_opaque, AddressSpace *as)
{
return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
translate_fn, translate_opaque, as);
}
/* Load a ramdisk. */
-int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
+ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
{
return load_ramdisk_as(filename, addr, max_sz, NULL);
}
-int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
- AddressSpace *as)
+ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
+ AddressSpace *as)
{
return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
NULL, NULL, as);
}
/* Load a gzip-compressed kernel to a dynamically allocated buffer. */
-int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
- uint8_t **buffer)
+ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
+ uint8_t **buffer)
{
uint8_t *compressed_data = NULL;
uint8_t *data = NULL;
@@ -824,9 +825,9 @@ int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
}
/* Load a gzip-compressed kernel. */
-int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
+ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
{
- int bytes;
+ ssize_t bytes;
uint8_t *data;
bytes = load_image_gzipped_buffer(filename, max_sz, &data);
@@ -956,14 +957,15 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
return data;
}
-int rom_add_file(const char *file, const char *fw_dir,
- hwaddr addr, int32_t bootindex,
- bool option_rom, MemoryRegion *mr,
- AddressSpace *as)
+ssize_t rom_add_file(const char *file, const char *fw_dir,
+ hwaddr addr, int32_t bootindex,
+ bool option_rom, MemoryRegion *mr,
+ AddressSpace *as)
{
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
Rom *rom;
- int rc, fd = -1;
+ ssize_t rc;
+ int fd = -1;
char devpath[100];
if (as && mr) {
@@ -1005,7 +1007,7 @@ int rom_add_file(const char *file, const char *fw_dir,
lseek(fd, 0, SEEK_SET);
rc = read(fd, rom->data, rom->datasize);
if (rc != rom->datasize) {
- fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
+ fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
rom->name, rc, rom->datasize);
goto err;
}
@@ -1124,12 +1126,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
return 0;
}
-int rom_add_vga(const char *file)
+ssize_t rom_add_vga(const char *file)
{
return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
}
-int rom_add_option(const char *file, int32_t bootindex)
+ssize_t rom_add_option(const char *file, int32_t bootindex)
{
return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
}
@@ -1742,11 +1744,12 @@ out:
}
/* return size or -1 if error */
-int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
+ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
+ AddressSpace *as)
{
gsize hex_blob_size;
gchar *hex_blob;
- int total_size = 0;
+ ssize_t total_size = 0;
if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
return -1;
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb99..1edf7ac53dfd 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1113,7 +1113,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
char *filename;
MemoryRegion *bios, *isa_bios;
int bios_size, isa_bios_size;
- int ret;
+ ssize_t ret;
/* BIOS load */
bios_name = ms->firmware ?: default_firmware;
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 519fa455a154..7d221db051bf 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -127,7 +127,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
hwaddr firmware_load_addr,
symbol_fn_t sym_cb)
{
- uint64_t firmware_entry, firmware_size, firmware_end;
+ uint64_t firmware_entry, firmware_end;
+ ssize_t firmware_size;
if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
&firmware_entry, NULL, &firmware_end, NULL,
@@ -176,7 +177,7 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
uint64_t kernel_entry, hwaddr *start)
{
- int size;
+ ssize_t size;
/*
* We want to put the initrd far enough into RAM that when the
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 4fa485bd61c7..a5e2925040c0 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
*
* Returns the size of the loaded image on success, -1 otherwise.
*/
-int load_image_targphys_as(const char *filename,
- hwaddr addr, uint64_t max_sz, AddressSpace *as);
+ssize_t load_image_targphys_as(const char *filename,
+ hwaddr addr, uint64_t max_sz, AddressSpace *as);
/**load_targphys_hex_as:
* @filename: Path to the .hex file
@@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
*
* Returns the size of the loaded .hex file on success, -1 otherwise.
*/
-int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as);
+ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
+ AddressSpace *as);
/** load_image_targphys:
* Same as load_image_targphys_as(), but doesn't allow the caller to specify
* an AddressSpace.
*/
-int load_image_targphys(const char *filename, hwaddr,
- uint64_t max_sz);
+ssize_t load_image_targphys(const char *filename, hwaddr,
+ uint64_t max_sz);
/**
* load_image_mr: load an image into a memory region
@@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
* If the file is larger than the memory region's size the call will fail.
* Returns -1 on failure, or the size of the file.
*/
-int load_image_mr(const char *filename, MemoryRegion *mr);
+ssize_t load_image_mr(const char *filename, MemoryRegion *mr);
/* This is the limit on the maximum uncompressed image size that
* load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
@@ -81,9 +82,9 @@ int load_image_mr(const char *filename, MemoryRegion *mr);
*/
#define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
-int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
- uint8_t **buffer);
-int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
+ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
+ uint8_t **buffer);
+ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
#define ELF_LOAD_FAILED -1
#define ELF_LOAD_NOT_ELF -2
@@ -183,8 +184,8 @@ ssize_t load_elf(const char *filename,
*/
void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
-int load_aout(const char *filename, hwaddr addr, int max_sz,
- int bswap_needed, hwaddr target_page_size);
+ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
+ int bswap_needed, hwaddr target_page_size);
#define LOAD_UIMAGE_LOADADDR_INVALID (-1)
@@ -205,19 +206,19 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
*
* Returns the size of the loaded image on success, -1 otherwise.
*/
-int load_uimage_as(const char *filename, hwaddr *ep,
- hwaddr *loadaddr, int *is_linux,
- uint64_t (*translate_fn)(void *, uint64_t),
- void *translate_opaque, AddressSpace *as);
+ssize_t load_uimage_as(const char *filename, hwaddr *ep,
+ hwaddr *loadaddr, int *is_linux,
+ uint64_t (*translate_fn)(void *, uint64_t),
+ void *translate_opaque, AddressSpace *as);
/** load_uimage:
* Same as load_uimage_as(), but doesn't allow the caller to specify an
* AddressSpace.
*/
-int load_uimage(const char *filename, hwaddr *ep,
- hwaddr *loadaddr, int *is_linux,
- uint64_t (*translate_fn)(void *, uint64_t),
- void *translate_opaque);
+ssize_t load_uimage(const char *filename, hwaddr *ep,
+ hwaddr *loadaddr, int *is_linux,
+ uint64_t (*translate_fn)(void *, uint64_t),
+ void *translate_opaque);
/**
* load_ramdisk_as:
@@ -232,15 +233,15 @@ int load_uimage(const char *filename, hwaddr *ep,
*
* Returns the size of the loaded image on success, -1 otherwise.
*/
-int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
- AddressSpace *as);
+ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
+ AddressSpace *as);
/**
* load_ramdisk:
* Same as load_ramdisk_as(), but doesn't allow the caller to specify
* an AddressSpace.
*/
-int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
+ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
@@ -253,9 +254,9 @@ void pstrcpy_targphys(const char *name,
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,
- bool option_rom, MemoryRegion *mr, AddressSpace *as);
+ssize_t rom_add_file(const char *file, const char *fw_dir,
+ hwaddr addr, 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,
const char *fw_file_name,
@@ -336,8 +337,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
#define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
-int rom_add_vga(const char *file);
-int rom_add_option(const char *file, int32_t bootindex);
+ssize_t rom_add_vga(const char *file);
+ssize_t rom_add_option(const char *file, int32_t bootindex);
/* This is the usual maximum in uboot, so if a uImage overflows this, it would
* overflow on real hardware too. */
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 14:11 [PATCH 0/2] Fix integer overflows in loading of large images Jamie Iles
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
@ 2021-11-11 14:11 ` Jamie Iles
2021-11-11 14:55 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 13+ messages in thread
From: Jamie Iles @ 2021-11-11 14:11 UTC (permalink / raw)
To: qemu-devel; +Cc: Jamie Iles, lmichel
On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
regardless of what is asked. If the file is larger than 0x7ffff000
bytes the read will need to be broken up into multiple chunks.
Cc: Luc Michel <lmichel@kalray.eu>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---
hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 348bbf535bd9..16ca9b99cf0f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
return size;
}
+static ssize_t read_large(int fd, void *dst, size_t len)
+{
+ /*
+ * man 2 read says:
+ *
+ * On Linux, read() (and similar system calls) will transfer at most
+ * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
+ * actually transferred. (This is true on both 32-bit and 64-bit
+ * systems.)
+ *
+ * So read in chunks no larger than 0x7ffff000 bytes.
+ */
+ size_t max_chunk_size = 0x7ffff000;
+ size_t offset = 0;
+
+ while (offset < len) {
+ size_t chunk_len = MIN(max_chunk_size, len - offset);
+ ssize_t br = read(fd, dst + offset, chunk_len);
+
+ if (br < 0) {
+ return br;
+ }
+ offset += br;
+ }
+
+ return (ssize_t)len;
+}
+
/* return the size or -1 if error */
ssize_t load_image_size(const char *filename, void *addr, size_t size)
{
@@ -91,7 +119,7 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
return -1;
}
- while ((actsize = read(fd, addr + l, size - l)) > 0) {
+ while ((actsize = read_large(fd, addr + l, size - l)) > 0) {
l += actsize;
}
@@ -108,7 +136,7 @@ ssize_t read_targphys(const char *name,
ssize_t did;
buf = g_malloc(nbytes);
- did = read(fd, buf, nbytes);
+ did = read_large(fd, buf, nbytes);
if (did > 0)
rom_add_blob_fixed("read", buf, did, dst_addr);
g_free(buf);
@@ -235,7 +263,7 @@ ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
if (fd < 0)
return -1;
- size = read(fd, &e, sizeof(e));
+ size = read_large(fd, &e, sizeof(e));
if (size < 0)
goto fail;
@@ -286,7 +314,7 @@ static void *load_at(int fd, off_t offset, size_t size)
if (lseek(fd, offset, SEEK_SET) < 0)
return NULL;
ptr = g_malloc(size);
- if (read(fd, ptr, size) != size) {
+ if (read_large(fd, ptr, size) != size) {
g_free(ptr);
return NULL;
}
@@ -714,7 +742,7 @@ static ssize_t load_uboot_image(const char *filename, hwaddr *ep,
data = g_malloc(hdr->ih_size);
- if (read(fd, data, hdr->ih_size) != hdr->ih_size) {
+ if (read_large(fd, data, hdr->ih_size) != hdr->ih_size) {
fprintf(stderr, "Error reading file\n");
goto out;
}
@@ -1005,7 +1033,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
rom->datasize = rom->romsize;
rom->data = g_malloc0(rom->datasize);
lseek(fd, 0, SEEK_SET);
- rc = read(fd, rom->data, rom->datasize);
+ rc = read_large(fd, rom->data, rom->datasize);
if (rc != rom->datasize) {
fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
rom->name, rc, rom->datasize);
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
@ 2021-11-11 14:20 ` Philippe Mathieu-Daudé
2021-11-12 8:25 ` Luc Michel
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 14:20 UTC (permalink / raw)
To: Jamie Iles, qemu-devel; +Cc: lmichel
On 11/11/21 15:11, Jamie Iles wrote:
> Various loader functions return an int which limits images to 2GB which
> is fine for things like a BIOS/kernel image, but if we want to be able
> to load memory images or large ramdisks then any file over 2GB would
> silently fail to load.
>
> Cc: Luc Michel <lmichel@kalray.eu>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> ---
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c | 8 ++--
> hw/core/generic-loader.c | 2 +-
> hw/core/loader.c | 81 +++++++++++++++++++++-------------------
> hw/i386/x86.c | 2 +-
> hw/riscv/boot.c | 5 ++-
> include/hw/loader.h | 55 +++++++++++++--------------
> 7 files changed, 80 insertions(+), 75 deletions(-)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 14:11 ` [PATCH 2/2] hw/core/loader: workaround read() size limit Jamie Iles
@ 2021-11-11 14:55 ` Philippe Mathieu-Daudé
2021-11-11 15:36 ` Jamie Iles
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 14:55 UTC (permalink / raw)
To: Jamie Iles, qemu-devel; +Cc: lmichel
Hi Jamie,
On 11/11/21 15:11, Jamie Iles wrote:
> On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
> regardless of what is asked. If the file is larger than 0x7ffff000
> bytes the read will need to be broken up into multiple chunks.
>
> Cc: Luc Michel <lmichel@kalray.eu>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> ---
> hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 348bbf535bd9..16ca9b99cf0f 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> return size;
> }
>
> +static ssize_t read_large(int fd, void *dst, size_t len)
> +{
> + /*
> + * man 2 read says:
> + *
> + * On Linux, read() (and similar system calls) will transfer at most
> + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
Could you mention MAX_RW_COUNT from linux/fs.h?
> + * actually transferred. (This is true on both 32-bit and 64-bit
> + * systems.)
Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
(because that would not be the case for the ILP64 model).
Otherwise s/systems/Linux variants/?
> + *
> + * So read in chunks no larger than 0x7ffff000 bytes.
> + */
> + size_t max_chunk_size = 0x7ffff000;
We can declare it static const.
> + size_t offset = 0;
> +
> + while (offset < len) {
> + size_t chunk_len = MIN(max_chunk_size, len - offset);
> + ssize_t br = read(fd, dst + offset, chunk_len);
> +
> + if (br < 0) {
> + return br;
> + }
> + offset += br;
> + }
> +
> + return (ssize_t)len;
> +}
I see other read()/pread() calls:
hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
region->fd_offset + addr) != size) {
...
Maybe the read_large() belongs to "sysemu/os-xxx.h"?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 14:55 ` Philippe Mathieu-Daudé
@ 2021-11-11 15:36 ` Jamie Iles
2021-11-11 15:43 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: Jamie Iles @ 2021-11-11 15:36 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Jamie Iles, qemu-devel, lmichel
Hi Philippe,
On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Jamie,
>
> On 11/11/21 15:11, Jamie Iles wrote:
> > On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
> > regardless of what is asked. If the file is larger than 0x7ffff000
> > bytes the read will need to be broken up into multiple chunks.
> >
> > Cc: Luc Michel <lmichel@kalray.eu>
> > Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> > ---
> > hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
> > 1 file changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index 348bbf535bd9..16ca9b99cf0f 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> > return size;
> > }
> >
> > +static ssize_t read_large(int fd, void *dst, size_t len)
> > +{
> > + /*
> > + * man 2 read says:
> > + *
> > + * On Linux, read() (and similar system calls) will transfer at most
> > + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
>
> Could you mention MAX_RW_COUNT from linux/fs.h?
>
> > + * actually transferred. (This is true on both 32-bit and 64-bit
> > + * systems.)
>
> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> (because that would not be the case for the ILP64 model).
>
> Otherwise s/systems/Linux variants/?
>
> > + *
> > + * So read in chunks no larger than 0x7ffff000 bytes.
> > + */
> > + size_t max_chunk_size = 0x7ffff000;
>
> We can declare it static const.
Ack, can fix all of those up.
> > + size_t offset = 0;
> > +
> > + while (offset < len) {
> > + size_t chunk_len = MIN(max_chunk_size, len - offset);
> > + ssize_t br = read(fd, dst + offset, chunk_len);
> > +
> > + if (br < 0) {
> > + return br;
> > + }
> > + offset += br;
> > + }
> > +
> > + return (ssize_t)len;
> > +}
>
> I see other read()/pread() calls:
>
> hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
> hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
> region->fd_offset + addr) != size) {
> ...
>
> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
I think util/osdep.c would be a good fit for this. To make sure we're
on the same page though are you proposing converting all pread/read
calls to a qemu variant or auditing for ones that could potentially take
a larger size?
Thanks,
Jamie
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 15:36 ` Jamie Iles
@ 2021-11-11 15:43 ` Philippe Mathieu-Daudé
2021-11-11 15:55 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 15:43 UTC (permalink / raw)
To: Jamie Iles; +Cc: qemu-devel, lmichel
On 11/11/21 16:36, Jamie Iles wrote:
> Hi Philippe,
>
> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Jamie,
>>
>> On 11/11/21 15:11, Jamie Iles wrote:
>>> On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
>>> regardless of what is asked. If the file is larger than 0x7ffff000
>>> bytes the read will need to be broken up into multiple chunks.
>>>
>>> Cc: Luc Michel <lmichel@kalray.eu>
>>> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
>>> ---
>>> hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>> index 348bbf535bd9..16ca9b99cf0f 100644
>>> --- a/hw/core/loader.c
>>> +++ b/hw/core/loader.c
>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
>>> return size;
>>> }
>>>
>>> +static ssize_t read_large(int fd, void *dst, size_t len)
>>> +{
>>> + /*
>>> + * man 2 read says:
>>> + *
>>> + * On Linux, read() (and similar system calls) will transfer at most
>>> + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
>>
>> Could you mention MAX_RW_COUNT from linux/fs.h?
>>
>>> + * actually transferred. (This is true on both 32-bit and 64-bit
>>> + * systems.)
>>
>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
>> (because that would not be the case for the ILP64 model).
>>
>> Otherwise s/systems/Linux variants/?
>>
>>> + *
>>> + * So read in chunks no larger than 0x7ffff000 bytes.
>>> + */
>>> + size_t max_chunk_size = 0x7ffff000;
>>
>> We can declare it static const.
>
> Ack, can fix all of those up.
>
>>> + size_t offset = 0;
>>> +
>>> + while (offset < len) {
>>> + size_t chunk_len = MIN(max_chunk_size, len - offset);
>>> + ssize_t br = read(fd, dst + offset, chunk_len);
>>> +
>>> + if (br < 0) {
>>> + return br;
>>> + }
>>> + offset += br;
>>> + }
>>> +
>>> + return (ssize_t)len;
>>> +}
>>
>> I see other read()/pread() calls:
>>
>> hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
>> hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
>> region->fd_offset + addr) != size) {
>> ...
>>
>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
>
> I think util/osdep.c would be a good fit for this. To make sure we're
Yes.
> on the same page though are you proposing converting all pread/read
> calls to a qemu variant or auditing for ones that could potentially take
> a larger size?
Yes, I took some time wondering beside loading blob in guest memory,
what would be the other issues you might encounter. I couldn't find
many cases. Eventually hw/vfio/. I haven't audit much, only noticed
hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
since we want to fix this, I'd rather try to fix it globally.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 15:43 ` Philippe Mathieu-Daudé
@ 2021-11-11 15:55 ` Philippe Mathieu-Daudé
2021-11-11 17:04 ` Jamie Iles
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-11 15:55 UTC (permalink / raw)
To: Jamie Iles; +Cc: qemu-devel, lmichel
On 11/11/21 16:43, Philippe Mathieu-Daudé wrote:
> On 11/11/21 16:36, Jamie Iles wrote:
>> Hi Philippe,
>>
>> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
>>> Hi Jamie,
>>>
>>> On 11/11/21 15:11, Jamie Iles wrote:
>>>> On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
>>>> regardless of what is asked. If the file is larger than 0x7ffff000
>>>> bytes the read will need to be broken up into multiple chunks.
>>>>
>>>> Cc: Luc Michel <lmichel@kalray.eu>
>>>> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
>>>> ---
>>>> hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 34 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>>>> index 348bbf535bd9..16ca9b99cf0f 100644
>>>> --- a/hw/core/loader.c
>>>> +++ b/hw/core/loader.c
>>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
>>>> return size;
>>>> }
>>>>
>>>> +static ssize_t read_large(int fd, void *dst, size_t len)
>>>> +{
>>>> + /*
>>>> + * man 2 read says:
>>>> + *
>>>> + * On Linux, read() (and similar system calls) will transfer at most
>>>> + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
>>>
>>> Could you mention MAX_RW_COUNT from linux/fs.h?
>>>
>>>> + * actually transferred. (This is true on both 32-bit and 64-bit
>>>> + * systems.)
>>>
>>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
>>> (because that would not be the case for the ILP64 model).
>>>
>>> Otherwise s/systems/Linux variants/?
>>>
>>>> + *
>>>> + * So read in chunks no larger than 0x7ffff000 bytes.
>>>> + */
>>>> + size_t max_chunk_size = 0x7ffff000;
>>>
>>> We can declare it static const.
>>
>> Ack, can fix all of those up.
>>
>>>> + size_t offset = 0;
>>>> +
>>>> + while (offset < len) {
>>>> + size_t chunk_len = MIN(max_chunk_size, len - offset);
>>>> + ssize_t br = read(fd, dst + offset, chunk_len);
>>>> +
>>>> + if (br < 0) {
>>>> + return br;
>>>> + }
>>>> + offset += br;
>>>> + }
>>>> +
>>>> + return (ssize_t)len;
>>>> +}
>>>
>>> I see other read()/pread() calls:
>>>
>>> hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
>>> hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
>>> region->fd_offset + addr) != size) {
>>> ...
>>>
>>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
>>
>> I think util/osdep.c would be a good fit for this. To make sure we're
>
> Yes.
>
>> on the same page though are you proposing converting all pread/read
>> calls to a qemu variant or auditing for ones that could potentially take
>> a larger size?
>
> Yes, I took some time wondering beside loading blob in guest memory,
> what would be the other issues you might encounter. I couldn't find
> many cases. Eventually hw/vfio/. I haven't audit much, only noticed
> hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
> since we want to fix this, I'd rather try to fix it globally.
Actually what you suggest is simpler, add qemu_read() / qemu_pread()
in util/osdep.c, convert all uses without caring about any audit.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 15:55 ` Philippe Mathieu-Daudé
@ 2021-11-11 17:04 ` Jamie Iles
2021-11-30 15:38 ` Jamie Iles
0 siblings, 1 reply; 13+ messages in thread
From: Jamie Iles @ 2021-11-11 17:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Jamie Iles, qemu-devel, lmichel
On Thu, Nov 11, 2021 at 04:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/11/21 16:43, Philippe Mathieu-Daudé wrote:
> > On 11/11/21 16:36, Jamie Iles wrote:
> >> Hi Philippe,
> >>
> >> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> >>> Hi Jamie,
> >>>
> >>> On 11/11/21 15:11, Jamie Iles wrote:
> >>>> On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
> >>>> regardless of what is asked. If the file is larger than 0x7ffff000
> >>>> bytes the read will need to be broken up into multiple chunks.
> >>>>
> >>>> Cc: Luc Michel <lmichel@kalray.eu>
> >>>> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> >>>> ---
> >>>> hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
> >>>> 1 file changed, 34 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >>>> index 348bbf535bd9..16ca9b99cf0f 100644
> >>>> --- a/hw/core/loader.c
> >>>> +++ b/hw/core/loader.c
> >>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> >>>> return size;
> >>>> }
> >>>>
> >>>> +static ssize_t read_large(int fd, void *dst, size_t len)
> >>>> +{
> >>>> + /*
> >>>> + * man 2 read says:
> >>>> + *
> >>>> + * On Linux, read() (and similar system calls) will transfer at most
> >>>> + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
> >>>
> >>> Could you mention MAX_RW_COUNT from linux/fs.h?
> >>>
> >>>> + * actually transferred. (This is true on both 32-bit and 64-bit
> >>>> + * systems.)
> >>>
> >>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> >>> (because that would not be the case for the ILP64 model).
> >>>
> >>> Otherwise s/systems/Linux variants/?
> >>>
> >>>> + *
> >>>> + * So read in chunks no larger than 0x7ffff000 bytes.
> >>>> + */
> >>>> + size_t max_chunk_size = 0x7ffff000;
> >>>
> >>> We can declare it static const.
> >>
> >> Ack, can fix all of those up.
> >>
> >>>> + size_t offset = 0;
> >>>> +
> >>>> + while (offset < len) {
> >>>> + size_t chunk_len = MIN(max_chunk_size, len - offset);
> >>>> + ssize_t br = read(fd, dst + offset, chunk_len);
> >>>> +
> >>>> + if (br < 0) {
> >>>> + return br;
> >>>> + }
> >>>> + offset += br;
> >>>> + }
> >>>> +
> >>>> + return (ssize_t)len;
> >>>> +}
> >>>
> >>> I see other read()/pread() calls:
> >>>
> >>> hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
> >>> hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
> >>> region->fd_offset + addr) != size) {
> >>> ...
> >>>
> >>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
> >>
> >> I think util/osdep.c would be a good fit for this. To make sure we're
> >
> > Yes.
> >
> >> on the same page though are you proposing converting all pread/read
> >> calls to a qemu variant or auditing for ones that could potentially take
> >> a larger size?
> >
> > Yes, I took some time wondering beside loading blob in guest memory,
> > what would be the other issues you might encounter. I couldn't find
> > many cases. Eventually hw/vfio/. I haven't audit much, only noticed
> > hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
> > since we want to fix this, I'd rather try to fix it globally.
>
> Actually what you suggest is simpler, add qemu_read() / qemu_pread()
> in util/osdep.c, convert all uses without caring about any audit.
Okay, this hasn't worked out too badly - I'll do the same for
write/pwrite too and then switch all of the callers over with a
coccinelle patch so it'll be a fairly large diff but simple.
We could elect to keep any calls with a compile-time constant length
with the unwrapped variants but I think that's probably more confusing
in the long-run.
Thanks,
Jamie
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
2021-11-11 14:20 ` Philippe Mathieu-Daudé
@ 2021-11-12 8:25 ` Luc Michel
2021-11-15 4:24 ` Alistair Francis
2022-06-02 1:13 ` Alistair Francis
3 siblings, 0 replies; 13+ messages in thread
From: Luc Michel @ 2021-11-12 8:25 UTC (permalink / raw)
To: Jamie Iles; +Cc: qemu-devel
On 14:11 Thu 11 Nov , Jamie Iles wrote:
> Various loader functions return an int which limits images to 2GB which
> is fine for things like a BIOS/kernel image, but if we want to be able
> to load memory images or large ramdisks then any file over 2GB would
> silently fail to load.
>
> Cc: Luc Michel <lmichel@kalray.eu>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
Reviewed-by: Luc Michel <lmichel@kalray.eu>
> ---
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c | 8 ++--
> hw/core/generic-loader.c | 2 +-
> hw/core/loader.c | 81 +++++++++++++++++++++-------------------
> hw/i386/x86.c | 2 +-
> hw/riscv/boot.c | 5 ++-
> include/hw/loader.h | 55 +++++++++++++--------------
> 7 files changed, 80 insertions(+), 75 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 8d08db80be83..a6393dce7276 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
>
> void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
> {
> - int image_size;
> + ssize_t image_size;
> uint64_t entry;
> int big_endian;
> AddressSpace *as;
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 74ad397b1ff9..3853203438ba 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
> return 0;
> }
>
> -static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> +static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, AddressSpace *as)
> {
> @@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> } elf_header;
> int data_swab = 0;
> bool big_endian;
> - int64_t ret = -1;
> + ssize_t ret = -1;
> Error *err = NULL;
>
>
> @@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> /* Set up for a direct boot of a kernel image file. */
> CPUState *cs;
> AddressSpace *as = arm_boot_address_space(cpu, info);
> - int kernel_size;
> + ssize_t kernel_size;
> int initrd_size;
> int is_linux = 0;
> uint64_t elf_entry;
> @@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>
> if (kernel_size > info->ram_size) {
> error_report("kernel '%s' is too large to fit in RAM "
> - "(kernel size %d, RAM size %" PRId64 ")",
> + "(kernel size %zd, RAM size %" PRId64 ")",
> info->kernel_filename, kernel_size, info->ram_size);
> exit(1);
> }
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea2e..bc1451da8f55 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> GenericLoaderState *s = GENERIC_LOADER(dev);
> hwaddr entry;
> int big_endian;
> - int size = 0;
> + ssize_t size = 0;
>
> s->set_pc = false;
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 052a0fd7198b..348bbf535bd9 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
> return did;
> }
>
> -int load_image_targphys(const char *filename,
> - hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_targphys(const char *filename,
> + hwaddr addr, uint64_t max_sz)
> {
> return load_image_targphys_as(filename, addr, max_sz, NULL);
> }
>
> /* return the size or -1 if error */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as)
> {
> - int size;
> + ssize_t size;
>
> size = get_image_size(filename);
> if (size < 0 || size > max_sz) {
> @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
> return size;
> }
>
> -int load_image_mr(const char *filename, MemoryRegion *mr)
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
> {
> - int size;
> + ssize_t size;
>
> if (!memory_access_is_direct(mr, false)) {
> /* Can only load an image into RAM or ROM */
> @@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
> : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), target_page_size)))
>
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size)
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size)
> {
> int fd;
> ssize_t size, ret;
> @@ -618,13 +618,14 @@ toosmall:
> }
>
> /* Load a U-Boot image. */
> -static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux, uint8_t image_type,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +static ssize_t load_uboot_image(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint8_t image_type,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> int fd;
> - int size;
> + ssize_t size;
> hwaddr address;
> uboot_image_header_t h;
> uboot_image_header_t *hdr = &h;
> @@ -746,40 +747,40 @@ out:
> return ret;
> }
>
> -int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque)
> +ssize_t load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, NULL);
> }
>
> -int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, as);
> }
>
> /* Load a ramdisk. */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> return load_ramdisk_as(filename, addr, max_sz, NULL);
> }
>
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as)
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as)
> {
> return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
> NULL, NULL, as);
> }
>
> /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer)
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer)
> {
> uint8_t *compressed_data = NULL;
> uint8_t *data = NULL;
> @@ -824,9 +825,9 @@ int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> }
>
> /* Load a gzip-compressed kernel. */
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> - int bytes;
> + ssize_t bytes;
> uint8_t *data;
>
> bytes = load_image_gzipped_buffer(filename, max_sz, &data);
> @@ -956,14 +957,15 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
> return data;
> }
>
> -int rom_add_file(const char *file, const char *fw_dir,
> - hwaddr addr, int32_t bootindex,
> - bool option_rom, MemoryRegion *mr,
> - AddressSpace *as)
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, int32_t bootindex,
> + bool option_rom, MemoryRegion *mr,
> + AddressSpace *as)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> - int rc, fd = -1;
> + ssize_t rc;
> + int fd = -1;
> char devpath[100];
>
> if (as && mr) {
> @@ -1005,7 +1007,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> lseek(fd, 0, SEEK_SET);
> rc = read(fd, rom->data, rom->datasize);
> if (rc != rom->datasize) {
> - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
> + fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> @@ -1124,12 +1126,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
> return 0;
> }
>
> -int rom_add_vga(const char *file)
> +ssize_t rom_add_vga(const char *file)
> {
> return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
> }
>
> -int rom_add_option(const char *file, int32_t bootindex)
> +ssize_t rom_add_option(const char *file, int32_t bootindex)
> {
> return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
> }
> @@ -1742,11 +1744,12 @@ out:
> }
>
> /* return size or -1 if error */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as)
> {
> gsize hex_blob_size;
> gchar *hex_blob;
> - int total_size = 0;
> + ssize_t total_size = 0;
>
> if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
> return -1;
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb99..1edf7ac53dfd 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1113,7 +1113,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
> char *filename;
> MemoryRegion *bios, *isa_bios;
> int bios_size, isa_bios_size;
> - int ret;
> + ssize_t ret;
>
> /* BIOS load */
> bios_name = ms->firmware ?: default_firmware;
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 519fa455a154..7d221db051bf 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -127,7 +127,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> symbol_fn_t sym_cb)
> {
> - uint64_t firmware_entry, firmware_size, firmware_end;
> + uint64_t firmware_entry, firmware_end;
> + ssize_t firmware_size;
>
> if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
> &firmware_entry, NULL, &firmware_end, NULL,
> @@ -176,7 +177,7 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
> hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> uint64_t kernel_entry, hwaddr *start)
> {
> - int size;
> + ssize_t size;
>
> /*
> * We want to put the initrd far enough into RAM that when the
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4fa485bd61c7..a5e2925040c0 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as);
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as);
>
> /**load_targphys_hex_as:
> * @filename: Path to the .hex file
> @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
> *
> * Returns the size of the loaded .hex file on success, -1 otherwise.
> */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as);
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as);
>
> /** load_image_targphys:
> * Same as load_image_targphys_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_image_targphys(const char *filename, hwaddr,
> - uint64_t max_sz);
> +ssize_t load_image_targphys(const char *filename, hwaddr,
> + uint64_t max_sz);
>
> /**
> * load_image_mr: load an image into a memory region
> @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
> * If the file is larger than the memory region's size the call will fail.
> * Returns -1 on failure, or the size of the file.
> */
> -int load_image_mr(const char *filename, MemoryRegion *mr);
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr);
>
> /* This is the limit on the maximum uncompressed image size that
> * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
> @@ -81,9 +82,9 @@ int load_image_mr(const char *filename, MemoryRegion *mr);
> */
> #define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
>
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer);
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer);
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>
> #define ELF_LOAD_FAILED -1
> #define ELF_LOAD_NOT_ELF -2
> @@ -183,8 +184,8 @@ ssize_t load_elf(const char *filename,
> */
> void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size);
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size);
>
> #define LOAD_UIMAGE_LOADADDR_INVALID (-1)
>
> @@ -205,19 +206,19 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_uimage_as(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as);
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as);
>
> /** load_uimage:
> * Same as load_uimage_as(), but doesn't allow the caller to specify an
> * AddressSpace.
> */
> -int load_uimage(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque);
> +ssize_t load_uimage(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque);
>
> /**
> * load_ramdisk_as:
> @@ -232,15 +233,15 @@ int load_uimage(const char *filename, hwaddr *ep,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as);
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as);
>
> /**
> * load_ramdisk:
> * Same as load_ramdisk_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
>
> ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
>
> @@ -253,9 +254,9 @@ void pstrcpy_targphys(const char *name,
> 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,
> - bool option_rom, MemoryRegion *mr, AddressSpace *as);
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, 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,
> const char *fw_file_name,
> @@ -336,8 +337,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
> rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>
> -int rom_add_vga(const char *file);
> -int rom_add_option(const char *file, int32_t bootindex);
> +ssize_t rom_add_vga(const char *file);
> +ssize_t rom_add_option(const char *file, int32_t bootindex);
>
> /* This is the usual maximum in uboot, so if a uImage overflows this, it would
> * overflow on real hardware too. */
> --
> 2.30.2
>
>
>
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=c2c2.618d24ab.74431.0&r=lmichel%40kalray.eu&s=jamie%40nuviainc.com&o=%5BPATCH+1%2F2%5D+hw%2Fcore%2Floader%3A+return+image+sizes+as+ssize_t&verdict=C&c=c9348b7ec2a7b51fa2790d170537816f09136839
>
--
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
2021-11-11 14:20 ` Philippe Mathieu-Daudé
2021-11-12 8:25 ` Luc Michel
@ 2021-11-15 4:24 ` Alistair Francis
2022-06-02 1:13 ` Alistair Francis
3 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2021-11-15 4:24 UTC (permalink / raw)
To: Jamie Iles; +Cc: qemu-devel@nongnu.org Developers, lmichel
On Fri, Nov 12, 2021 at 12:12 AM Jamie Iles <jamie@nuviainc.com> wrote:
>
> Various loader functions return an int which limits images to 2GB which
> is fine for things like a BIOS/kernel image, but if we want to be able
> to load memory images or large ramdisks then any file over 2GB would
> silently fail to load.
>
> Cc: Luc Michel <lmichel@kalray.eu>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c | 8 ++--
> hw/core/generic-loader.c | 2 +-
> hw/core/loader.c | 81 +++++++++++++++++++++-------------------
> hw/i386/x86.c | 2 +-
> hw/riscv/boot.c | 5 ++-
> include/hw/loader.h | 55 +++++++++++++--------------
> 7 files changed, 80 insertions(+), 75 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 8d08db80be83..a6393dce7276 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
>
> void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
> {
> - int image_size;
> + ssize_t image_size;
> uint64_t entry;
> int big_endian;
> AddressSpace *as;
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 74ad397b1ff9..3853203438ba 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
> return 0;
> }
>
> -static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> +static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, AddressSpace *as)
> {
> @@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> } elf_header;
> int data_swab = 0;
> bool big_endian;
> - int64_t ret = -1;
> + ssize_t ret = -1;
> Error *err = NULL;
>
>
> @@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> /* Set up for a direct boot of a kernel image file. */
> CPUState *cs;
> AddressSpace *as = arm_boot_address_space(cpu, info);
> - int kernel_size;
> + ssize_t kernel_size;
> int initrd_size;
> int is_linux = 0;
> uint64_t elf_entry;
> @@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>
> if (kernel_size > info->ram_size) {
> error_report("kernel '%s' is too large to fit in RAM "
> - "(kernel size %d, RAM size %" PRId64 ")",
> + "(kernel size %zd, RAM size %" PRId64 ")",
> info->kernel_filename, kernel_size, info->ram_size);
> exit(1);
> }
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea2e..bc1451da8f55 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> GenericLoaderState *s = GENERIC_LOADER(dev);
> hwaddr entry;
> int big_endian;
> - int size = 0;
> + ssize_t size = 0;
>
> s->set_pc = false;
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 052a0fd7198b..348bbf535bd9 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
> return did;
> }
>
> -int load_image_targphys(const char *filename,
> - hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_targphys(const char *filename,
> + hwaddr addr, uint64_t max_sz)
> {
> return load_image_targphys_as(filename, addr, max_sz, NULL);
> }
>
> /* return the size or -1 if error */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as)
> {
> - int size;
> + ssize_t size;
>
> size = get_image_size(filename);
> if (size < 0 || size > max_sz) {
> @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
> return size;
> }
>
> -int load_image_mr(const char *filename, MemoryRegion *mr)
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
> {
> - int size;
> + ssize_t size;
>
> if (!memory_access_is_direct(mr, false)) {
> /* Can only load an image into RAM or ROM */
> @@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
> : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), target_page_size)))
>
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size)
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size)
> {
> int fd;
> ssize_t size, ret;
> @@ -618,13 +618,14 @@ toosmall:
> }
>
> /* Load a U-Boot image. */
> -static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux, uint8_t image_type,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +static ssize_t load_uboot_image(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint8_t image_type,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> int fd;
> - int size;
> + ssize_t size;
> hwaddr address;
> uboot_image_header_t h;
> uboot_image_header_t *hdr = &h;
> @@ -746,40 +747,40 @@ out:
> return ret;
> }
>
> -int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque)
> +ssize_t load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, NULL);
> }
>
> -int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, as);
> }
>
> /* Load a ramdisk. */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> return load_ramdisk_as(filename, addr, max_sz, NULL);
> }
>
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as)
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as)
> {
> return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
> NULL, NULL, as);
> }
>
> /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer)
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer)
> {
> uint8_t *compressed_data = NULL;
> uint8_t *data = NULL;
> @@ -824,9 +825,9 @@ int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> }
>
> /* Load a gzip-compressed kernel. */
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> - int bytes;
> + ssize_t bytes;
> uint8_t *data;
>
> bytes = load_image_gzipped_buffer(filename, max_sz, &data);
> @@ -956,14 +957,15 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
> return data;
> }
>
> -int rom_add_file(const char *file, const char *fw_dir,
> - hwaddr addr, int32_t bootindex,
> - bool option_rom, MemoryRegion *mr,
> - AddressSpace *as)
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, int32_t bootindex,
> + bool option_rom, MemoryRegion *mr,
> + AddressSpace *as)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> - int rc, fd = -1;
> + ssize_t rc;
> + int fd = -1;
> char devpath[100];
>
> if (as && mr) {
> @@ -1005,7 +1007,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> lseek(fd, 0, SEEK_SET);
> rc = read(fd, rom->data, rom->datasize);
> if (rc != rom->datasize) {
> - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
> + fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> @@ -1124,12 +1126,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
> return 0;
> }
>
> -int rom_add_vga(const char *file)
> +ssize_t rom_add_vga(const char *file)
> {
> return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
> }
>
> -int rom_add_option(const char *file, int32_t bootindex)
> +ssize_t rom_add_option(const char *file, int32_t bootindex)
> {
> return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
> }
> @@ -1742,11 +1744,12 @@ out:
> }
>
> /* return size or -1 if error */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as)
> {
> gsize hex_blob_size;
> gchar *hex_blob;
> - int total_size = 0;
> + ssize_t total_size = 0;
>
> if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
> return -1;
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb99..1edf7ac53dfd 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1113,7 +1113,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
> char *filename;
> MemoryRegion *bios, *isa_bios;
> int bios_size, isa_bios_size;
> - int ret;
> + ssize_t ret;
>
> /* BIOS load */
> bios_name = ms->firmware ?: default_firmware;
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 519fa455a154..7d221db051bf 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -127,7 +127,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> symbol_fn_t sym_cb)
> {
> - uint64_t firmware_entry, firmware_size, firmware_end;
> + uint64_t firmware_entry, firmware_end;
> + ssize_t firmware_size;
>
> if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
> &firmware_entry, NULL, &firmware_end, NULL,
> @@ -176,7 +177,7 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
> hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> uint64_t kernel_entry, hwaddr *start)
> {
> - int size;
> + ssize_t size;
>
> /*
> * We want to put the initrd far enough into RAM that when the
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4fa485bd61c7..a5e2925040c0 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as);
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as);
>
> /**load_targphys_hex_as:
> * @filename: Path to the .hex file
> @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
> *
> * Returns the size of the loaded .hex file on success, -1 otherwise.
> */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as);
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as);
>
> /** load_image_targphys:
> * Same as load_image_targphys_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_image_targphys(const char *filename, hwaddr,
> - uint64_t max_sz);
> +ssize_t load_image_targphys(const char *filename, hwaddr,
> + uint64_t max_sz);
>
> /**
> * load_image_mr: load an image into a memory region
> @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
> * If the file is larger than the memory region's size the call will fail.
> * Returns -1 on failure, or the size of the file.
> */
> -int load_image_mr(const char *filename, MemoryRegion *mr);
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr);
>
> /* This is the limit on the maximum uncompressed image size that
> * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
> @@ -81,9 +82,9 @@ int load_image_mr(const char *filename, MemoryRegion *mr);
> */
> #define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
>
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer);
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer);
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>
> #define ELF_LOAD_FAILED -1
> #define ELF_LOAD_NOT_ELF -2
> @@ -183,8 +184,8 @@ ssize_t load_elf(const char *filename,
> */
> void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size);
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size);
>
> #define LOAD_UIMAGE_LOADADDR_INVALID (-1)
>
> @@ -205,19 +206,19 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_uimage_as(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as);
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as);
>
> /** load_uimage:
> * Same as load_uimage_as(), but doesn't allow the caller to specify an
> * AddressSpace.
> */
> -int load_uimage(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque);
> +ssize_t load_uimage(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque);
>
> /**
> * load_ramdisk_as:
> @@ -232,15 +233,15 @@ int load_uimage(const char *filename, hwaddr *ep,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as);
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as);
>
> /**
> * load_ramdisk:
> * Same as load_ramdisk_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
>
> ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
>
> @@ -253,9 +254,9 @@ void pstrcpy_targphys(const char *name,
> 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,
> - bool option_rom, MemoryRegion *mr, AddressSpace *as);
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, 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,
> const char *fw_file_name,
> @@ -336,8 +337,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
> rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>
> -int rom_add_vga(const char *file);
> -int rom_add_option(const char *file, int32_t bootindex);
> +ssize_t rom_add_vga(const char *file);
> +ssize_t rom_add_option(const char *file, int32_t bootindex);
>
> /* This is the usual maximum in uboot, so if a uImage overflows this, it would
> * overflow on real hardware too. */
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
2021-11-11 17:04 ` Jamie Iles
@ 2021-11-30 15:38 ` Jamie Iles
0 siblings, 0 replies; 13+ messages in thread
From: Jamie Iles @ 2021-11-30 15:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, lmichel
Hi Philippe,
On Thu, Nov 11, 2021 at 05:04:56PM +0000, Jamie Iles wrote:
> On Thu, Nov 11, 2021 at 04:55:35PM +0100, Philippe Mathieu-Daudé wrote:
> > On 11/11/21 16:43, Philippe Mathieu-Daudé wrote:
> > > On 11/11/21 16:36, Jamie Iles wrote:
> > >> Hi Philippe,
> > >>
> > >> On Thu, Nov 11, 2021 at 03:55:48PM +0100, Philippe Mathieu-Daudé wrote:
> > >>> Hi Jamie,
> > >>>
> > >>> On 11/11/21 15:11, Jamie Iles wrote:
> > >>>> On Linux, read() will only ever read a maximum of 0x7ffff000 bytes
> > >>>> regardless of what is asked. If the file is larger than 0x7ffff000
> > >>>> bytes the read will need to be broken up into multiple chunks.
> > >>>>
> > >>>> Cc: Luc Michel <lmichel@kalray.eu>
> > >>>> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> > >>>> ---
> > >>>> hw/core/loader.c | 40 ++++++++++++++++++++++++++++++++++------
> > >>>> 1 file changed, 34 insertions(+), 6 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/core/loader.c b/hw/core/loader.c
> > >>>> index 348bbf535bd9..16ca9b99cf0f 100644
> > >>>> --- a/hw/core/loader.c
> > >>>> +++ b/hw/core/loader.c
> > >>>> @@ -80,6 +80,34 @@ int64_t get_image_size(const char *filename)
> > >>>> return size;
> > >>>> }
> > >>>>
> > >>>> +static ssize_t read_large(int fd, void *dst, size_t len)
> > >>>> +{
> > >>>> + /*
> > >>>> + * man 2 read says:
> > >>>> + *
> > >>>> + * On Linux, read() (and similar system calls) will transfer at most
> > >>>> + * 0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
> > >>>
> > >>> Could you mention MAX_RW_COUNT from linux/fs.h?
> > >>>
> > >>>> + * actually transferred. (This is true on both 32-bit and 64-bit
> > >>>> + * systems.)
> > >>>
> > >>> Maybe "This is true for both ILP32 and LP64 data models used by Linux"?
> > >>> (because that would not be the case for the ILP64 model).
> > >>>
> > >>> Otherwise s/systems/Linux variants/?
> > >>>
> > >>>> + *
> > >>>> + * So read in chunks no larger than 0x7ffff000 bytes.
> > >>>> + */
> > >>>> + size_t max_chunk_size = 0x7ffff000;
> > >>>
> > >>> We can declare it static const.
> > >>
> > >> Ack, can fix all of those up.
> > >>
> > >>>> + size_t offset = 0;
> > >>>> +
> > >>>> + while (offset < len) {
> > >>>> + size_t chunk_len = MIN(max_chunk_size, len - offset);
> > >>>> + ssize_t br = read(fd, dst + offset, chunk_len);
> > >>>> +
> > >>>> + if (br < 0) {
> > >>>> + return br;
> > >>>> + }
> > >>>> + offset += br;
> > >>>> + }
> > >>>> +
> > >>>> + return (ssize_t)len;
> > >>>> +}
> > >>>
> > >>> I see other read()/pread() calls:
> > >>>
> > >>> hw/9pfs/9p-local.c:472: tsize = read(fd, (void *)buf, bufsz);
> > >>> hw/vfio/common.c:269: if (pread(vbasedev->fd, &buf, size,
> > >>> region->fd_offset + addr) != size) {
> > >>> ...
> > >>>
> > >>> Maybe the read_large() belongs to "sysemu/os-xxx.h"?
> > >>
> > >> I think util/osdep.c would be a good fit for this. To make sure we're
> > >
> > > Yes.
> > >
> > >> on the same page though are you proposing converting all pread/read
> > >> calls to a qemu variant or auditing for ones that could potentially take
> > >> a larger size?
> > >
> > > Yes, I took some time wondering beside loading blob in guest memory,
> > > what would be the other issues you might encounter. I couldn't find
> > > many cases. Eventually hw/vfio/. I haven't audit much, only noticed
> > > hw/9pfs/9p-local.c and qga/commands-*.c (not sure if relevant), but
> > > since we want to fix this, I'd rather try to fix it globally.
> >
> > Actually what you suggest is simpler, add qemu_read() / qemu_pread()
> > in util/osdep.c, convert all uses without caring about any audit.
>
> Okay, this hasn't worked out too badly - I'll do the same for
> write/pwrite too and then switch all of the callers over with a
> coccinelle patch so it'll be a fairly large diff but simple.
>
> We could elect to keep any calls with a compile-time constant length
> with the unwrapped variants but I think that's probably more confusing
> in the long-run.
Coming back to this I think this is probably a non-starter because of
non-blocking file descriptors. There is already a qemu_write_full so
I'm inclined to add qemu_read_full following the same pattern and then
convert all of the read calls in the loader to use that.
Thanks,
Jamie
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
` (2 preceding siblings ...)
2021-11-15 4:24 ` Alistair Francis
@ 2022-06-02 1:13 ` Alistair Francis
3 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2022-06-02 1:13 UTC (permalink / raw)
To: Jamie Iles; +Cc: qemu-devel@nongnu.org Developers, lmichel
On Fri, Nov 12, 2021 at 12:12 AM Jamie Iles <jamie@nuviainc.com> wrote:
>
> Various loader functions return an int which limits images to 2GB which
> is fine for things like a BIOS/kernel image, but if we want to be able
> to load memory images or large ramdisks then any file over 2GB would
> silently fail to load.
>
> Cc: Luc Michel <lmichel@kalray.eu>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c | 8 ++--
> hw/core/generic-loader.c | 2 +-
> hw/core/loader.c | 81 +++++++++++++++++++++-------------------
> hw/i386/x86.c | 2 +-
> hw/riscv/boot.c | 5 ++-
> include/hw/loader.h | 55 +++++++++++++--------------
> 7 files changed, 80 insertions(+), 75 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 8d08db80be83..a6393dce7276 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
>
> void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
> {
> - int image_size;
> + ssize_t image_size;
> uint64_t entry;
> int big_endian;
> AddressSpace *as;
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 74ad397b1ff9..3853203438ba 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
> return 0;
> }
>
> -static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> +static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, AddressSpace *as)
> {
> @@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> } elf_header;
> int data_swab = 0;
> bool big_endian;
> - int64_t ret = -1;
> + ssize_t ret = -1;
> Error *err = NULL;
>
>
> @@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> /* Set up for a direct boot of a kernel image file. */
> CPUState *cs;
> AddressSpace *as = arm_boot_address_space(cpu, info);
> - int kernel_size;
> + ssize_t kernel_size;
> int initrd_size;
> int is_linux = 0;
> uint64_t elf_entry;
> @@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>
> if (kernel_size > info->ram_size) {
> error_report("kernel '%s' is too large to fit in RAM "
> - "(kernel size %d, RAM size %" PRId64 ")",
> + "(kernel size %zd, RAM size %" PRId64 ")",
> info->kernel_filename, kernel_size, info->ram_size);
> exit(1);
> }
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea2e..bc1451da8f55 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> GenericLoaderState *s = GENERIC_LOADER(dev);
> hwaddr entry;
> int big_endian;
> - int size = 0;
> + ssize_t size = 0;
>
> s->set_pc = false;
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 052a0fd7198b..348bbf535bd9 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
> return did;
> }
>
> -int load_image_targphys(const char *filename,
> - hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_targphys(const char *filename,
> + hwaddr addr, uint64_t max_sz)
> {
> return load_image_targphys_as(filename, addr, max_sz, NULL);
> }
>
> /* return the size or -1 if error */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as)
> {
> - int size;
> + ssize_t size;
>
> size = get_image_size(filename);
> if (size < 0 || size > max_sz) {
> @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
> return size;
> }
>
> -int load_image_mr(const char *filename, MemoryRegion *mr)
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
> {
> - int size;
> + ssize_t size;
>
> if (!memory_access_is_direct(mr, false)) {
> /* Can only load an image into RAM or ROM */
> @@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
> : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size), target_page_size)))
>
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size)
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size)
> {
> int fd;
> ssize_t size, ret;
> @@ -618,13 +618,14 @@ toosmall:
> }
>
> /* Load a U-Boot image. */
> -static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux, uint8_t image_type,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +static ssize_t load_uboot_image(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint8_t image_type,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> int fd;
> - int size;
> + ssize_t size;
> hwaddr address;
> uboot_image_header_t h;
> uboot_image_header_t *hdr = &h;
> @@ -746,40 +747,40 @@ out:
> return ret;
> }
>
> -int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque)
> +ssize_t load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, NULL);
> }
>
> -int load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> - int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as)
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as)
> {
> return load_uboot_image(filename, ep, loadaddr, is_linux, IH_TYPE_KERNEL,
> translate_fn, translate_opaque, as);
> }
>
> /* Load a ramdisk. */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> return load_ramdisk_as(filename, addr, max_sz, NULL);
> }
>
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as)
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as)
> {
> return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK,
> NULL, NULL, as);
> }
>
> /* Load a gzip-compressed kernel to a dynamically allocated buffer. */
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer)
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer)
> {
> uint8_t *compressed_data = NULL;
> uint8_t *data = NULL;
> @@ -824,9 +825,9 @@ int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> }
>
> /* Load a gzip-compressed kernel. */
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz)
> {
> - int bytes;
> + ssize_t bytes;
> uint8_t *data;
>
> bytes = load_image_gzipped_buffer(filename, max_sz, &data);
> @@ -956,14 +957,15 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
> return data;
> }
>
> -int rom_add_file(const char *file, const char *fw_dir,
> - hwaddr addr, int32_t bootindex,
> - bool option_rom, MemoryRegion *mr,
> - AddressSpace *as)
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, int32_t bootindex,
> + bool option_rom, MemoryRegion *mr,
> + AddressSpace *as)
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> - int rc, fd = -1;
> + ssize_t rc;
> + int fd = -1;
> char devpath[100];
>
> if (as && mr) {
> @@ -1005,7 +1007,7 @@ int rom_add_file(const char *file, const char *fw_dir,
> lseek(fd, 0, SEEK_SET);
> rc = read(fd, rom->data, rom->datasize);
> if (rc != rom->datasize) {
> - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
> + fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> @@ -1124,12 +1126,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
> return 0;
> }
>
> -int rom_add_vga(const char *file)
> +ssize_t rom_add_vga(const char *file)
> {
> return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
> }
>
> -int rom_add_option(const char *file, int32_t bootindex)
> +ssize_t rom_add_option(const char *file, int32_t bootindex)
> {
> return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
> }
> @@ -1742,11 +1744,12 @@ out:
> }
>
> /* return size or -1 if error */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as)
> {
> gsize hex_blob_size;
> gchar *hex_blob;
> - int total_size = 0;
> + ssize_t total_size = 0;
>
> if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) {
> return -1;
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb99..1edf7ac53dfd 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -1113,7 +1113,7 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
> char *filename;
> MemoryRegion *bios, *isa_bios;
> int bios_size, isa_bios_size;
> - int ret;
> + ssize_t ret;
>
> /* BIOS load */
> bios_name = ms->firmware ?: default_firmware;
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 519fa455a154..7d221db051bf 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -127,7 +127,8 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> symbol_fn_t sym_cb)
> {
> - uint64_t firmware_entry, firmware_size, firmware_end;
> + uint64_t firmware_entry, firmware_end;
> + ssize_t firmware_size;
>
> if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
> &firmware_entry, NULL, &firmware_end, NULL,
> @@ -176,7 +177,7 @@ target_ulong riscv_load_kernel(const char *kernel_filename,
> hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> uint64_t kernel_entry, hwaddr *start)
> {
> - int size;
> + ssize_t size;
>
> /*
> * We want to put the initrd far enough into RAM that when the
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4fa485bd61c7..a5e2925040c0 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size);
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as);
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace *as);
>
> /**load_targphys_hex_as:
> * @filename: Path to the .hex file
> @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
> *
> * Returns the size of the loaded .hex file on success, -1 otherwise.
> */
> -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as);
> +ssize_t load_targphys_hex_as(const char *filename, hwaddr *entry,
> + AddressSpace *as);
>
> /** load_image_targphys:
> * Same as load_image_targphys_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_image_targphys(const char *filename, hwaddr,
> - uint64_t max_sz);
> +ssize_t load_image_targphys(const char *filename, hwaddr,
> + uint64_t max_sz);
>
> /**
> * load_image_mr: load an image into a memory region
> @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
> * If the file is larger than the memory region's size the call will fail.
> * Returns -1 on failure, or the size of the file.
> */
> -int load_image_mr(const char *filename, MemoryRegion *mr);
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr);
>
> /* This is the limit on the maximum uncompressed image size that
> * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
> @@ -81,9 +82,9 @@ int load_image_mr(const char *filename, MemoryRegion *mr);
> */
> #define LOAD_IMAGE_MAX_GUNZIP_BYTES (256 << 20)
>
> -int load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> - uint8_t **buffer);
> -int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_image_gzipped_buffer(const char *filename, uint64_t max_sz,
> + uint8_t **buffer);
> +ssize_t load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>
> #define ELF_LOAD_FAILED -1
> #define ELF_LOAD_NOT_ELF -2
> @@ -183,8 +184,8 @@ ssize_t load_elf(const char *filename,
> */
> void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size);
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size);
>
> #define LOAD_UIMAGE_LOADADDR_INVALID (-1)
>
> @@ -205,19 +206,19 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_uimage_as(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque, AddressSpace *as);
> +ssize_t load_uimage_as(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque, AddressSpace *as);
>
> /** load_uimage:
> * Same as load_uimage_as(), but doesn't allow the caller to specify an
> * AddressSpace.
> */
> -int load_uimage(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux,
> - uint64_t (*translate_fn)(void *, uint64_t),
> - void *translate_opaque);
> +ssize_t load_uimage(const char *filename, hwaddr *ep,
> + hwaddr *loadaddr, int *is_linux,
> + uint64_t (*translate_fn)(void *, uint64_t),
> + void *translate_opaque);
>
> /**
> * load_ramdisk_as:
> @@ -232,15 +233,15 @@ int load_uimage(const char *filename, hwaddr *ep,
> *
> * Returns the size of the loaded image on success, -1 otherwise.
> */
> -int load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> - AddressSpace *as);
> +ssize_t load_ramdisk_as(const char *filename, hwaddr addr, uint64_t max_sz,
> + AddressSpace *as);
>
> /**
> * load_ramdisk:
> * Same as load_ramdisk_as(), but doesn't allow the caller to specify
> * an AddressSpace.
> */
> -int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
> +ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
>
> ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
>
> @@ -253,9 +254,9 @@ void pstrcpy_targphys(const char *name,
> 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,
> - bool option_rom, MemoryRegion *mr, AddressSpace *as);
> +ssize_t rom_add_file(const char *file, const char *fw_dir,
> + hwaddr addr, 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,
> const char *fw_file_name,
> @@ -336,8 +337,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \
> rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
>
> -int rom_add_vga(const char *file);
> -int rom_add_option(const char *file, int32_t bootindex);
> +ssize_t rom_add_vga(const char *file);
> +ssize_t rom_add_option(const char *file, int32_t bootindex);
>
> /* This is the usual maximum in uboot, so if a uImage overflows this, it would
> * overflow on real hardware too. */
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-06-02 1:17 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 14:11 [PATCH 0/2] Fix integer overflows in loading of large images Jamie Iles
2021-11-11 14:11 ` [PATCH 1/2] hw/core/loader: return image sizes as ssize_t Jamie Iles
2021-11-11 14:20 ` Philippe Mathieu-Daudé
2021-11-12 8:25 ` Luc Michel
2021-11-15 4:24 ` Alistair Francis
2022-06-02 1:13 ` Alistair Francis
2021-11-11 14:11 ` [PATCH 2/2] hw/core/loader: workaround read() size limit Jamie Iles
2021-11-11 14:55 ` Philippe Mathieu-Daudé
2021-11-11 15:36 ` Jamie Iles
2021-11-11 15:43 ` Philippe Mathieu-Daudé
2021-11-11 15:55 ` Philippe Mathieu-Daudé
2021-11-11 17:04 ` Jamie Iles
2021-11-30 15:38 ` Jamie Iles
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.