[Qemu-devel] S390: file size checking in load_image_targphys and certain ram sizes

[Qemu-devel] S390: file size checking in load_image_targphys and certain ram sizes

[Christian Borntraeger]

Ben, Alex,

commit 17df768c1e4580f03301d18ea938d3557d441911
    load_image_targphys() should enforce the max size

caused some problems with external kernel and specific ram sizes on s390:

We load the external kernel with

[...]
            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
[...]

The problem is now, that load_image_targphys has max_sz as an int (32bit), but 
ram_size is a ram_addr_t (64bit).
So for a ramsize of lets say 3GB the comparison in load_image_targphys fails:

    if (size > max_sz) {
        return -1;
    }

There are several potential ways of solving, suggestions for a better solution
than the patch below are welcome.



diff --git a/hw/loader.c b/hw/loader.c
index 415cdce..8a6c99d 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -103,7 +103,7 @@ 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, uint64_t max_sz)
 {
     int size;
 
diff --git a/hw/loader.h b/hw/loader.h
index fbcaba9..5cfa6df 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -4,7 +4,7 @@
 /* loader.c */
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
-int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
+int load_image_targphys(const char *filename, target_phys_addr_t, uint64_t);
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
              uint64_t *highaddr, int big_endian, int elf_machine,

Re: [Qemu-devel] S390: file size checking in load_image_targphys and certain ram sizes

[Alexander Graf]

On 03.05.2012, at 15:36, Christian Borntraeger wrote:

> 
> Ben, Alex,
> 
> commit 17df768c1e4580f03301d18ea938d3557d441911
>    load_image_targphys() should enforce the max size
> 
> caused some problems with external kernel and specific ram sizes on s390:
> 
> We load the external kernel with
> 
> [...]
>            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
> [...]
> 
> The problem is now, that load_image_targphys has max_sz as an int (32bit), but 
> ram_size is a ram_addr_t (64bit).
> So for a ramsize of lets say 3GB the comparison in load_image_targphys fails:
> 
>    if (size > max_sz) {
>        return -1;
>    }
> 
> There are several potential ways of solving, suggestions for a better solution
> than the patch below are welcome.

The proposed solution looks perfectly fine. In fact I remember having had a very similar discussion on an ARM issue and the conclusion we came up with was to use the biggest available data type: uint64_t.

Pleas repost this as a proper patch with Signed-off-by :).


Alex

[Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Christian Borntraeger]

commit 17df768c1e4580f03301d18ea938d3557d441911
    load_image_targphys() should enforce the max size

caused some problems with external kernel and specific ram sizes on s390:

We load the external kernel with

[...]
            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
[...]

Since load_image_targphys is declared as taking an int for max_sz, this will
fail for ram sizes > INT_MAX.
Lets change the max_sz parameter to a uint64_t.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/loader.c |    2 +-
 hw/loader.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 415cdce..7d64113 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -103,7 +103,7 @@ 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, uint64_t max_sz)
 {
     int size;
 
diff --git a/hw/loader.h b/hw/loader.h
index fbcaba9..5cfa6df 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -4,7 +4,7 @@
 /* loader.c */
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
-int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
+int load_image_targphys(const char *filename, target_phys_addr_t, uint64_t);
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
              void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
              uint64_t *highaddr, int big_endian, int elf_machine,
-- 
1.7.9.6

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Peter Maydell]

On 3 May 2012 15:27, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> commit 17df768c1e4580f03301d18ea938d3557d441911
>    load_image_targphys() should enforce the max size
>
> caused some problems with external kernel and specific ram sizes on s390:
>
> We load the external kernel with
>
> [...]
>            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
> [...]
>
> Since load_image_targphys is declared as taking an int for max_sz, this will
> fail for ram sizes > INT_MAX.
> Lets change the max_sz parameter to a uint64_t.
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

A patch equivalent to this has already been submitted:
  http://patchwork.ozlabs.org/patch/146165/
We should be applying that one, it has already been reviewed.

Anthony?

-- PMM

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Christian Borntraeger]

>> Since load_image_targphys is declared as taking an int for max_sz, this will
>> fail for ram sizes > INT_MAX.
>> Lets change the max_sz parameter to a uint64_t.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> A patch equivalent to this has already been submitted:
>   http://patchwork.ozlabs.org/patch/146165/
> We should be applying that one, it has already been reviewed.

Right.

> 
> Anthony?

AFAIK Anthony is on holiday. It would be good, if we could have that before 1.1, so shall
we wait or are there other maintainers willing to accept that patch?

Christian

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Christian Borntraeger]

On 03/05/12 16:33, Peter Maydell wrote:
>> Since load_image_targphys is declared as taking an int for max_sz, this will
>> fail for ram sizes > INT_MAX.
>> Lets change the max_sz parameter to a uint64_t.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> A patch equivalent to this has already been submitted:
>   http://patchwork.ozlabs.org/patch/146165/
> We should be applying that one, it has already been reviewed.
> 
> Anthony?

Ping?

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Christian Borntraeger]

On 03/05/12 16:33, Peter Maydell wrote:
> On 3 May 2012 15:27, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>> commit 17df768c1e4580f03301d18ea938d3557d441911
>>    load_image_targphys() should enforce the max size
>>
>> caused some problems with external kernel and specific ram sizes on s390:
>>
>> We load the external kernel with
>>
>> [...]
>>            kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
>> [...]
>>
>> Since load_image_targphys is declared as taking an int for max_sz, this will
>> fail for ram sizes > INT_MAX.
>> Lets change the max_sz parameter to a uint64_t.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> A patch equivalent to this has already been submitted:
>   http://patchwork.ozlabs.org/patch/146165/
> We should be applying that one, it has already been reviewed.
> 
> Anthony?

Anthony,

can you apply the patchwork patch for 1.1?

Christian

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Alexander Graf]

On 16.05.2012, at 12:29, Christian Borntraeger wrote:

> On 03/05/12 16:33, Peter Maydell wrote:
>> On 3 May 2012 15:27, Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>> commit 17df768c1e4580f03301d18ea938d3557d441911
>>>  load_image_targphys() should enforce the max size
>>> 
>>> caused some problems with external kernel and specific ram sizes on s390:
>>> 
>>> We load the external kernel with
>>> 
>>> [...]
>>>          kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
>>> [...]
>>> 
>>> Since load_image_targphys is declared as taking an int for max_sz, this will
>>> fail for ram sizes > INT_MAX.
>>> Lets change the max_sz parameter to a uint64_t.
>>> 
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> 
>> A patch equivalent to this has already been submitted:
>> http://patchwork.ozlabs.org/patch/146165/
>> We should be applying that one, it has already been reviewed.
>> 
>> Anthony?
> 
> Anthony,
> 
> can you apply the patchwork patch for 1.1?

Ping? Still missing in rc2.


Alex

Re: [Qemu-devel] [PATCH] Fix size checking in load_image_targphys to accept max_size > INT_MAX

[Anthony Liguori]

On 05/17/2012 07:18 PM, Alexander Graf wrote:
>
> On 16.05.2012, at 12:29, Christian Borntraeger wrote:
>
>> On 03/05/12 16:33, Peter Maydell wrote:
>>> On 3 May 2012 15:27, Christian Borntraeger<borntraeger@de.ibm.com>  wrote:
>>>> commit 17df768c1e4580f03301d18ea938d3557d441911
>>>>   load_image_targphys() should enforce the max size
>>>>
>>>> caused some problems with external kernel and specific ram sizes on s390:
>>>>
>>>> We load the external kernel with
>>>>
>>>> [...]
>>>>           kernel_size = load_image_targphys(kernel_filename, 0, ram_size);
>>>> [...]
>>>>
>>>> Since load_image_targphys is declared as taking an int for max_sz, this will
>>>> fail for ram sizes>  INT_MAX.
>>>> Lets change the max_sz parameter to a uint64_t.
>>>>
>>>> Signed-off-by: Christian Borntraeger<borntraeger@de.ibm.com>
>>>
>>> A patch equivalent to this has already been submitted:
>>> http://patchwork.ozlabs.org/patch/146165/
>>> We should be applying that one, it has already been reviewed.
>>>
>>> Anthony?
>>
>> Anthony,
>>
>> can you apply the patchwork patch for 1.1?
>
> Ping? Still missing in rc2.

Ack.

Regards,

Anthony Liguori

>
>
> Alex
>