From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Fri, 22 Jun 2018 13:31:39 -0600 Subject: [U-Boot] [PATCH v8 17/30] sandbox: Enhance map_to_sysmem() to handle foreign pointers In-Reply-To: <9a1a67a1-c828-e8c3-20fd-701b5f51409d@suse.de> References: <20180618140835.195901-1-sjg@chromium.org> <20180618140835.195901-18-sjg@chromium.org> <82898ada-d1ce-35aa-032d-cb6d2a8d76b7@suse.de> <9a1a67a1-c828-e8c3-20fd-701b5f51409d@suse.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Alex, On 22 June 2018 at 06:12, Alexander Graf wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 04:00, Alexander Graf wrote: >>> >>> On 06/21/2018 04:01 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 18 June 2018 at 08:50, Alexander Graf 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 >>>>>> --- >>>>>> >>>>>> 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