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 <lmic...@kalray.eu>
> > >>>> Signed-off-by: Jamie Iles <ja...@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
