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

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

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

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.

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.

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