Patchwork arm highbank: force ramsize to INT_MAX when loading

login
register
mail settings
Submitter Mark Langsdorf
Date March 9, 2012, 3:57 p.m.
Message ID <1331308660-20787-1-git-send-email-mark.langsdorf@calxeda.com>
Download mbox | patch
Permalink /patch/145735/
State New
Headers show

Comments

Mark Langsdorf - March 9, 2012, 3:57 p.m.
Since the ram_size field of arm_boot_info is only an int, don't set
that field to more than INT_MAX. Signed vs unsigned comparison
overruns are possible otherwise.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
 hw/highbank.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter Maydell - March 9, 2012, 4:13 p.m.
On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
> Since the ram_size field of arm_boot_info is only an int, don't set
> that field to more than INT_MAX. Signed vs unsigned comparison
> overruns are possible otherwise.

Can't we just make arm_boot_info.ram_size a uint32_t (propagating through
signedness fixes as required) ?

Actually it should probably be a target_phys_addr_t, thinking ahead
to adding LPAE support.

-- PMM
Mark Langsdorf - March 9, 2012, 4:40 p.m.
On 03/09/2012 10:13 AM, Peter Maydell wrote:
> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>> Since the ram_size field of arm_boot_info is only an int, don't set
>> that field to more than INT_MAX. Signed vs unsigned comparison
>> overruns are possible otherwise.
> 
> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through
> signedness fixes as required) ?
> 
> Actually it should probably be a target_phys_addr_t, thinking ahead
> to adding LPAE support.

It really should be a size_t, per the upthread discussion with Andreas
Faerber.

I'll take a stab at the patch, but it touches a lot of code that I don't
really have a way to test so I'm a bit dubious.

--Mark Langsdorf
Calxeda, Inc.
Alexander Graf - March 9, 2012, 6:22 p.m.
On 09.03.2012, at 17:40, Mark Langsdorf wrote:

> On 03/09/2012 10:13 AM, Peter Maydell wrote:
>> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>>> Since the ram_size field of arm_boot_info is only an int, don't set
>>> that field to more than INT_MAX. Signed vs unsigned comparison
>>> overruns are possible otherwise.
>> 
>> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through
>> signedness fixes as required) ?
>> 
>> Actually it should probably be a target_phys_addr_t, thinking ahead
>> to adding LPAE support.
> 
> It really should be a size_t, per the upthread discussion with Andreas
> Faerber.

No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period.


Alex
Andreas Färber - March 9, 2012, 7:03 p.m.
Am 09.03.2012 19:22, schrieb Alexander Graf:
> 
> On 09.03.2012, at 17:40, Mark Langsdorf wrote:
> 
>> On 03/09/2012 10:13 AM, Peter Maydell wrote:
>>> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>>>> Since the ram_size field of arm_boot_info is only an int, don't set
>>>> that field to more than INT_MAX. Signed vs unsigned comparison
>>>> overruns are possible otherwise.
>>>
>>> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through
>>> signedness fixes as required) ?
>>>
>>> Actually it should probably be a target_phys_addr_t, thinking ahead
>>> to adding LPAE support.
>>
>> It really should be a size_t, per the upthread discussion with Andreas
>> Faerber.
> 
> No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period.

That is exactly where we disagree: It's not "target ram fiddling code".

It's the host loading binaries from disk to host RAM.

The Memory API constructs for wiring it up in guest RAM are outside the
scope of this discussion.

If we can't load a 4 GB file into our host RAM, there's no point
bloating our APIs with unreadable types as arguments such as
"target_phys_addr_t max_size" just because a 64-bit guest on a 32-bit
host has a larger virtual address space.

It's not an address, so if we end up needing a special type it should
certainly not be any ...addr_t type. Typedef target_phys_size_t if you
see a need but keep our APIs clean please. Alex, you say you don't care
about infrastructure, well this IS infrastructure and I care about its
API and usability. :)

Andreas
Alexander Graf - March 9, 2012, 7:21 p.m.
On 09.03.2012, at 20:03, Andreas Färber wrote:

> Am 09.03.2012 19:22, schrieb Alexander Graf:
>> 
>> On 09.03.2012, at 17:40, Mark Langsdorf wrote:
>> 
>>> On 03/09/2012 10:13 AM, Peter Maydell wrote:
>>>> On 9 March 2012 15:57, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>>>>> Since the ram_size field of arm_boot_info is only an int, don't set
>>>>> that field to more than INT_MAX. Signed vs unsigned comparison
>>>>> overruns are possible otherwise.
>>>> 
>>>> Can't we just make arm_boot_info.ram_size a uint32_t (propagating through
>>>> signedness fixes as required) ?
>>>> 
>>>> Actually it should probably be a target_phys_addr_t, thinking ahead
>>>> to adding LPAE support.
>>> 
>>> It really should be a size_t, per the upthread discussion with Andreas
>>> Faerber.
>> 
>> No, Andreas is wrong. Host data types have nothing to do in target ram fiddling code. The type you're searching for is "the size of physical address space the guest can handle" and that's target_phys_addr_t. Period.
> 
> That is exactly where we disagree: It's not "target ram fiddling code".
> 
> It's the host loading binaries from disk to host RAM.

No, it's an offset into guest RAM.

Right now the interface is using

  target_phys_addr_t addr
  int size

what if instead if would be using

  target_phys_addr_t start
  target_phys_addr_t end

It would still be the same thing semantically. We have a start address and an end address that we can put data in. The fact that the image is backed by some POSIX layers in the background is a minor detail. It could just as well go through the block layer and be read via libcurl from http.

The semantic of size is

  addr + size = end


Alex

Patch

diff --git a/hw/highbank.c b/hw/highbank.c
index 489c00e..577284a 100644
--- a/hw/highbank.c
+++ b/hw/highbank.c
@@ -306,7 +306,7 @@  static void highbank_init(ram_addr_t ram_size,
         sysbus_connect_irq(sysbus_from_qdev(dev), 2, pic[82]);
     }
 
-    highbank_binfo.ram_size = ram_size;
+    highbank_binfo.ram_size = (ram_size < INT_MAX ? ram_size : INT_MAX);
     highbank_binfo.kernel_filename = kernel_filename;
     highbank_binfo.kernel_cmdline = kernel_cmdline;
     highbank_binfo.initrd_filename = initrd_filename;