| Message ID | 20240627020207.630125-1-gregorhaas1997@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/core/loader: allow loading larger ROMs | expand |
Hi, Gregor > > The read() syscall is not guaranteed to return all data from a file. The > default ROM loader implementation currently does not take this into account, > instead failing if all bytes are not read at once. This change wraps the > read() syscall in a do/while loop to ensure all bytes of the ROM are read. Can you reproduce this issue? Thanks Xingtao
Hi Xingtao, > Can you reproduce this issue? Absolutely! I encountered this when trying to load an OpenSBI payload firmware using the bios option for the QEMU RISC-V virt board. These payload firmwares bundle the entire next boot stage, which in my case is a build of the Linux kernel (which is a standard configuration, supported by tools such as Buildroot [1]). My kernel (configured with the default 64-bit RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI firmware of final size 10M. Then, I run the following QEMU command: qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin and get the following output: rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) qemu-system-riscv64: could not load firmware 'fw_payload.bin' This is from my development machine, running Arch Linux with kernel 6.9.6 and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make a minimal reproducer for this, or if you need any more information. Thanks, Gregor [1] https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) < yaoxt.fnst@fujitsu.com> wrote: > Hi, Gregor > > > > The read() syscall is not guaranteed to return all data from a file. The > > default ROM loader implementation currently does not take this into > account, > > instead failing if all bytes are not read at once. This change wraps the > > read() syscall in a do/while loop to ensure all bytes of the ROM are > read. > Can you reproduce this issue? > > Thanks > Xingtao >
Hi, Gregor >rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) >qemu-system-riscv64: could not load firmware 'fw_payload.bin' Thanks, I was able to reproduce the problem when the images size is larger than 2147479552. I found that in my test environment, the maximum value returned by a read operation is 2147479552, which was affected by the operating system. We can find this limitation in the man page: NOTES The types size_t and ssize_t are, respectively, unsigned and signed integer data types specified by POSIX.1. 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.) > + do { > + rc = read(fd, &rom->data[sz], rom->datasize); > + if (rc == -1) { > + fprintf(stderr, "rom: file %-20s: read error: %s\n", > + rom->name, strerror(errno)); > + goto err; > + } > + sz += rc; > + } while (sz != rom->datasize); I think we can use load_image_size() instead. From: Gregor Haas <gregorhaas1997@gmail.com> Sent: Friday, June 28, 2024 1:35 AM To: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> Cc: qemu-devel@nongnu.org; philmd@linaro.org; richard.henderson@linaro.org Subject: Re: [PATCH] hw/core/loader: allow loading larger ROMs Hi Xingtao, > Can you reproduce this issue? Absolutely! I encountered this when trying to load an OpenSBI payload firmware using the bios option for the QEMU RISC-V virt board. These payload firmwares bundle the entire next boot stage, which in my case is a build of the Linux kernel (which is a standard configuration, supported by tools such as Buildroot [1]). My kernel (configured with the default 64-bit RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI firmware of final size 10M. Then, I run the following QEMU command: qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin and get the following output: rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) qemu-system-riscv64: could not load firmware 'fw_payload.bin' This is from my development machine, running Arch Linux with kernel 6.9.6 and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make a minimal reproducer for this, or if you need any more information. Thanks, Gregor [1] https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) <yaoxt.fnst@fujitsu.com<mailto:yaoxt.fnst@fujitsu.com>> wrote: Hi, Gregor > > The read() syscall is not guaranteed to return all data from a file. The > default ROM loader implementation currently does not take this into account, > instead failing if all bytes are not read at once. This change wraps the > read() syscall in a do/while loop to ensure all bytes of the ROM are read. Can you reproduce this issue? Thanks Xingtao
Hi Xingtao, Thank you for reproducing this -- I agree with your conclusion and will send a v2 patchset momentarily. Thank you, Gregor On Thu, Jun 27, 2024 at 5:44 PM Xingtao Yao (Fujitsu) < yaoxt.fnst@fujitsu.com> wrote: > Hi, Gregor > > > > >rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) > >qemu-system-riscv64: could not load firmware 'fw_payload.bin' > > Thanks, I was able to reproduce the problem when the images size is > larger than 2147479552. > > > > I found that in my test environment, the maximum value returned by a read > operation is 2147479552, > > which was affected by the operating system. > > > > We can find this limitation in the man page: > > NOTES > > The types size_t and ssize_t are, respectively, unsigned and > signed integer data types specified by POSIX.1. > > > > 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.) > > > > > > > + do { > > > + rc = read(fd, &rom->data[sz], rom->datasize); > > > + if (rc == -1) { > > > + fprintf(stderr, "rom: file %-20s: read error: %s\n", > > > + rom->name, strerror(errno)); > > > + goto err; > > > + } > > > + sz += rc; > > > + } while (sz != rom->datasize); > > I think we can use load_image_size() instead. > > > > > > > > > > *From:* Gregor Haas <gregorhaas1997@gmail.com> > *Sent:* Friday, June 28, 2024 1:35 AM > *To:* Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com> > *Cc:* qemu-devel@nongnu.org; philmd@linaro.org; > richard.henderson@linaro.org > *Subject:* Re: [PATCH] hw/core/loader: allow loading larger ROMs > > > > Hi Xingtao, > > > Can you reproduce this issue? > Absolutely! I encountered this when trying to load an OpenSBI payload > firmware using the bios option for the QEMU RISC-V virt board. These > payload firmwares bundle the entire next boot stage, which in my case is a > build of the Linux kernel (which is a standard configuration, supported by > tools such as Buildroot [1]). My kernel (configured with the default 64-bit > RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI > firmware of final size 10M. Then, I run the following QEMU command: > > qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin > > and get the following output: > > rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) > qemu-system-riscv64: could not load firmware 'fw_payload.bin' > > This is from my development machine, running Arch Linux with kernel 6.9.6 > and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make > a minimal reproducer for this, or if you need any more information. > > Thanks, > Gregor > > [1] > https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 > > > > On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) < > yaoxt.fnst@fujitsu.com> wrote: > > Hi, Gregor > > > > The read() syscall is not guaranteed to return all data from a file. The > > default ROM loader implementation currently does not take this into > account, > > instead failing if all bytes are not read at once. This change wraps the > > read() syscall in a do/while loop to ensure all bytes of the ROM are > read. > Can you reproduce this issue? > > Thanks > Xingtao > >
diff --git a/hw/core/loader.c b/hw/core/loader.c index 2f8105d7de..afa26dccb1 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1075,7 +1075,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir, { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; - ssize_t rc; + ssize_t rc, sz = 0; int fd = -1; char devpath[100]; @@ -1116,12 +1116,15 @@ 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); - if (rc != rom->datasize) { - fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n", - rom->name, rc, rom->datasize); - goto err; - } + do { + rc = read(fd, &rom->data[sz], rom->datasize); + if (rc == -1) { + fprintf(stderr, "rom: file %-20s: read error: %s\n", + rom->name, strerror(errno)); + goto err; + } + sz += rc; + } while (sz != rom->datasize); close(fd); rom_insert(rom); if (rom->fw_file && fw_cfg) {
The read() syscall is not guaranteed to return all data from a file. The default ROM loader implementation currently does not take this into account, instead failing if all bytes are not read at once. This change wraps the read() syscall in a do/while loop to ensure all bytes of the ROM are read. Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com> --- hw/core/loader.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)