[PATCH] hw/core/loader: allow loading larger ROMs

[PATCH] hw/core/loader: allow loading larger ROMs

[Gregor Haas]

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

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) {
-- 
2.45.2

RE: [PATCH] hw/core/loader: allow loading larger ROMs

[Xingtao Yao (Fujitsu) via]

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

Re: [PATCH] hw/core/loader: allow loading larger ROMs

[Gregor Haas]

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

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
>

[-- Attachment #2: Type: text/html, Size: 2100 bytes --]

RE: [PATCH] hw/core/loader: allow loading larger ROMs

[Xingtao Yao (Fujitsu) via]

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

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

[-- Attachment #2: Type: text/html, Size: 50555 bytes --]

Re: [PATCH] hw/core/loader: allow loading larger ROMs

[Gregor Haas]

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

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

[-- Attachment #2: Type: text/html, Size: 9695 bytes --]