Re: [Qemu-devel] [PATCH] use an unsigned long for the max_sz parameter in load_image_targphys

[Markus Armbruster]

Mark Langsdorf <address@hidden> writes:

> On 03/09/2012 03:25 AM, Markus Armbruster wrote:
>> Mark Langsdorf <address@hidden> writes:
>> 
>>> Allow load_image_targphys to load files on systems with more than 2G of
>>> emulated memory by changing the max_sz parameter from an int to an
>>> unsigned long.
>>>
>>> Signed-off-by: Mark Langsdorf <address@hidden>
>>> ---
>>>  hw/loader.c |    4 ++--
>>>  hw/loader.h |    3 ++-
>>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/loader.c b/hw/loader.c
>>> index 415cdce..a5333d0 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -103,9 +103,9 @@ ssize_t read_targphys(const char *name,
>>>  
>>>  /* return the size or -1 if error */
>>>  int load_image_targphys(const char *filename,
>>> -                   target_phys_addr_t addr, int max_sz)
>>> +                        target_phys_addr_t addr, unsigned long max_sz)
>>>  {
>>> -    int size;
>>> +    unsigned long size;
>>>  
>>>      size = get_image_size(filename);
>>>      if (size > max_sz) {
>> 
>> get_image_size() returns int.  How does widening size and max_sz here
>> improve things?
>
> If max_sz is greater than 2GB, then:
>   int max_sz = 0xc0000000;
>   int size =   0x300;
>   if (size > max_sz)
>       return -1;
>
> returns -1, even though size is much less than max_sz.
>
> doing it my way:
>   unsigned long max_sz = 0xc0000000;
>   unsigned long size =   0x300;
>   if (size > max_sz)
>       return -1;
>
> does not return -1.

The current code limits max_sz to INT_MAX.  Passing 0xc0000000 is a bug.

You want to relax the limit.  That's fair.  But I'm afraid your patch
doesn't really relax the limit, or rather it relaxes it just by one bit.

Your example shows it works for a case where the higher limit isn't
needed.  Let's examine three cases where it is: image sizes 2GiB, 4GiB
and 5GiB on a host with 32 bit twos complement int, and 64 bit unsigned
size_t, max_sz 0xc0000000.

Expected result: the first one works, the other two exceed max_sz and
get rejected.

Actual result, correct me if I'm wrong:

1. get_image_size() calls lseek(), which returns (off_t)2147483648 for
   2GiB, (off_t)4294967296 for 4GiB, and (off_t)5368709120 for 5GiB.

2. get_image_size() assigns it to int size.  Since int can't hold the
   value, the result is technically implementation-defined, but in
   practice it's simply -2147483648 for 2GiB, 0 for 4GiB, and 1073741824
   for 5GiB.

3. get_image_size() returns the same.

4. load_image_targphys() assigns the return value to unsigned long size.
   This changes the value back to 2147483648 for 2GiB, leaves it at 0
   for 4GiB, and at 1073741824 for 5GiB.

5. load_image_targphys() fails if size > max_sz.  Doesn't fail for any
   of the three.

6. load_image_targphys() adds the ROM file unless size is zero.  Adds
   the first and the last file.

   Bug: the second case's image is ignored silently, not rejected for
   exceeding max_sz.

7. rom_add_file() creates a Rom object.  It gets the image size again
   (hello TOCTTOU, not your patch's fault), and stores it in member
   size_t romsize.

   Bug: the third case's image is loaded even though it exceeds max_sz.
   Hello buffer overrun.

I'm afraid you need to do more work to solve this problem.  If you're
willing to do that, please check out
http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg03627.html