[U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:12, Alexander Graf <agraf at suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 04:00, Alexander Graf <agraf at suse.de> wrote:
>>>
>>> On 06/21/2018 04:01 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 18 June 2018 at 08:50, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>
>>>>>> At present map_sysmem() maps an address into the sandbox RAM buffer,
>>>>>> return a pointer, while map_to_sysmem() goes the other way.
>>>>>>
>>>>>> The mapping is currently just 1:1 since a case was not found where a
>>>>>> more
>>>>>> flexible mapping was needed. PCI does have a separate and more complex
>>>>>> mapping, but uses its own mechanism.
>>>>>>
>>>>>> However this arrange cannot handle one important case, which is where
>>>>>> a
>>>>>> test declares a stack variable and passes a pointer to it into a
>>>>>> U-Boot
>>>>>> function which uses map_to_sysmem() to turn it into a address. Since
>>>>>> the
>>>>>> pointer is not inside emulated DRAM, this will fail.
>>>>>>
>>>>>> Add a mapping feature which can handle any such pointer, mapping it to
>>>>>> a
>>>>>> simple tag value which can be passed around in U-Boot as an address.
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v8: None
>>>>>> Changes in v7: None
>>>>>> Changes in v6: None
>>>>>> Changes in v5: None
>>>>>> Changes in v4: None
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>>
>>>>>>     arch/sandbox/cpu/cpu.c           | 141
>>>>>> +++++++++++++++++++++++++++++--
>>>>>>     arch/sandbox/cpu/state.c         |   8 ++
>>>>>>     arch/sandbox/include/asm/state.h |  21 +++++
>>>>>>     3 files changed, 161 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>>>>> index cde0b055a6..fd77603ebf 100644
>>>>>> --- a/arch/sandbox/cpu/cpu.c
>>>>>> +++ b/arch/sandbox/cpu/cpu.c
>>>>>> @@ -57,14 +57,104 @@ int cleanup_before_linux_select(int flags)
>>>>>>           return 0;
>>>>>>     }
>>>>>>     +/**
>>>>>> + * is_in_sandbox_mem() - Checks if a pointer is within sandbox's
>>>>>> emulated
>>>>>> DRAM
>>>>>> + *
>>>>>> + * This provides a way to check if a pointer is owned by sandbox (and
>>>>>> is
>>>>>> within
>>>>>> + * its RAM) or not. Sometimes pointers come from a test which
>>>>>> conceptually runs
>>>>>> + * output sandbox, potentially with direct access to the C-library
>>>>>> malloc()
>>>>>> + * function, or the sandbox stack (which is not actually within the
>>>>>> emulated
>>>>>> + * DRAM.
>>>>>> + *
>>>>>> + * Such pointers obviously cannot be mapped into sandbox's DRAM, so
>>>>>> we
>>>>>> must
>>>>>> + * detect them an process them separately, by recording a mapping to
>>>>>> a
>>>>>> tag,
>>>>>> + * which we can use to map back to the pointer later.
>>>>>> + *
>>>>>> + * @ptr: Pointer to check
>>>>>> + * @return true if this is within sandbox emulated DRAM, false if not
>>>>>> + */
>>>>>> +static bool is_in_sandbox_mem(const void *ptr)
>>>>>> +{
>>>>>> +       return (const uint8_t *)ptr >= gd->arch.ram_buf &&
>>>>>> +               (const uint8_t *)ptr < gd->arch.ram_buf +
>>>>>> gd->ram_size;
>>>>>
>>>>>
>>>>> Greater/Less comparisons on pointers are invalid (albeit used) C IIRC.
>>>>> You
>>>>> should cast to uintptr_t instead and compare those.
>>>>
>>>> Reference? That is news to me :-)
>>>>
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * phys_to_virt() - Converts a sandbox RAM address to a pointer
>>>>>> + *
>>>>>> + * Sandbox uses U-Boot addresses from 0 to the size of DRAM. These
>>>>>> index
>>>>>> into
>>>>>> + * the emulated DRAM buffer used by sandbox. This function converts
>>>>>> such
>>>>>> an
>>>>>> + * address to a pointer into thi sbuffer, which can be used to access
>>>>>> the
>>>>>
>>>>>
>>>>> this
>>>>
>>>> OK fixed, ta.
>>>>
>>>>> ...
>>>>>
>>>>> Thinking about this, wouldn't it be easier to just allocate the stack
>>>>> inside
>>>>> U-Boot RAM?
>>>>
>>>> We could change the tests so that instead of using local variables
>>>> they call a function to allocate sandbox RAM. But I'm not sure it is
>>>> easier.
>>>
>>>
>>> No, what I'm saying is why don't we just in early bootup code move %sp to
>>> an
>>> address that resides inside the U-Boot address space? Then things would
>>> behave just like on real hardware.
>>
>> I was hoping that wasn't what you were saying!
>>
>> I worry about messing with the host system in that way. It seems
>> really gross. Is it safe?
>
>
> I think it's safe - coroutines do it all the time - but it would push us
> into writing architecture specific code for sandbox.

OK, well perhaps something to think about in the future.

>
> Either way, I don't particularly care whether you want to bloat up the
> mapping logic or swap stack pointers. This is sandbox only realm and you can
> do whatever you wish there ;)

To some extent, yes. But if we want to prevent 'foreign' pointers
coming into sandbox, we would need to handle bss, stack and the
rodata/data. I'm sure it could work, but it seems like it would make
sandbox into a rather strange program. I had trouble even with the
minor linker-script changes I have already done, on some
architectures.

Regards,
Simon