[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] hw/core/loader: workaround read() size limit.
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 2/2] hw/core/loader: workaround read() size limit. |
Date: |
Thu, 11 Nov 2021 16:55:35 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
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.
[PATCH 1/2] hw/core/loader: return image sizes as ssize_t, Jamie Iles, 2021/11/11