Add ARM userspace infrastructure #4974

agross-oss · 2017-11-14T21:39:36Z

This patch set adds the ARM based userspace infrastructure. The patches include all the parts to get system calls, demoting threads to user mode, etc.

andrewboie

I'm not too deeply familar with ARM internals but I do have some comments.

It's not clear to me how the kernel stack is set up, if this is being carved out of the existing thread stack, need to change this so that the kernel stack is reserved either completely separately (like on x86 where it is between the guard and the thread stack) or carved to a fixed size out of the thread stack, it shouldn't be a function of the thread stack size.

We don't know for sure how big the kernel stack needs to be, I'd suggest 2K as a reasonably safe choice and we can tune it later.

andrewboie · 2017-11-14T21:40:48Z

arch/arm/core/thread.c

@@ -67,22 +67,23 @@ void _new_thread(struct k_thread *thread, k_thread_stack_t *stack,
 	pInitCtx = (struct __esf *)(STACK_ROUND_DOWN(stackEnd -
 						     sizeof(struct __esf)));

+#if CONFIG_ARM_USERSPACE
+	if (!(options & K_USER)) {
+		pInitCtx->pc = ((u32_t)_thread_entry) & 0xfffffffe;


why the mask on the function pointer?

I figured it out. It's forcing the mode to ARM mode. If the LSB is 1, it means it's thumb mode.

andrewboie · 2017-11-14T21:44:47Z

arch/arm/core/userspace.S

+    mov lr,r0
+    push {r1,r2,r3,lr}
+
+    /* disable mpu stack guard to keep us from triggering guard region */


why is this needed?
On x86 we didn't need to touch the guard region at all.
Also please note, CONFIG_USERSPACE no longer depends on CONFIG_HW_STACK_PROTECTION and there may be no guard region.

yeah I just saw this. I'll have to fix this piece.

andrewboie · 2017-11-14T21:46:06Z

arch/arm/core/userspace.S

+    cmp r0,ip
+    blt valid_syscall
+    push {lr}
+    blx _do_kernel_oops


better to just override the system call id with K_SYSCALL_BAD and executing that entry in the dispatch table, with the bad ID as the first arg.
See x86 userspace.S

Yeah that is cleaner. I'll change it to that.

andrewboie · 2017-11-14T21:48:34Z

arch/arm/core/fatal.c

+
+FUNC_NORETURN void _arch_syscall_oops(void *ssf_ptr)
+{
+	_do_kernel_oops((const NANO_ESF *)ssf_ptr);


just wanted to double-check, is ssf_ptr really a NANO_ESF?
on x86 it wasn't and I had to copy stuff.

I'll double check.

i still have to fix this. I'll push another change tomorrow to address the stack frame. I think I'll have to copy the svc stack frame or reconstruct it from the pieces I have on hand (like the x86 method)

galak

some style comments

galak · 2017-11-14T21:48:48Z

arch/arm/core/thread.c

@@ -67,22 +67,23 @@ void _new_thread(struct k_thread *thread, k_thread_stack_t *stack,
 	pInitCtx = (struct __esf *)(STACK_ROUND_DOWN(stackEnd -
 						     sizeof(struct __esf)));

+#if CONFIG_ARM_USERSPACE
+	if (!(options & K_USER)) {


can we swap this so its:

if (options & K_USER) {
pInitCtx->pc = ((u32_t)_arch_user_mode_enter) & 0xfffffffe;
} else {
pInitCtx->pc = ((u32_t)_thread_entry) & 0xfffffffe;
}

Maybe setting pInitCtx->pc to either _arch_user_mode_enter or _thread_entry, and masking the bits after the #ifdef block will generate slightly smaller code.

galak · 2017-11-14T21:49:03Z

arch/arm/core/swap.S

@@ -324,6 +327,19 @@ _oops:
    blx _do_kernel_oops
    pop {pc}

+_escalate_priv:


should we ifdef protect this?

the pop {pc} is nefarious because it literally pops off the stack into the pc, and then voila, you are there. But yeah I can block this off.

galak · 2017-11-14T21:51:16Z

include/arch/arm/arch.h

-	return 0;
+	u32_t control;
+	__asm__ volatile("mrs %0, CONTROL\n\t" : "=r"(control));
+	return control & 0x1 ;


nit - remove space between 0x1 & ;

andrewboie · 2017-11-14T21:59:55Z

CI failures due to checkpatch being unhappy about whitespace

lpereira · 2017-11-14T21:58:48Z

include/arch/arm/arch.h

 }

 static inline int _arch_is_user_context(void)
 {
-	return 0;
+	u32_t control;


Minor style nit in this function: missing vertical space between variable declaration and after the assembly block, and there's a stray space before the semicolon in the return statement.

lpereira · 2017-11-14T22:01:11Z

include/arch/arm/arch.h

 */
+extern int _arm_do_syscall(u32_t call_id, u32_t arg1,


Will this work? call_id is the last argument passed by all the _arch_syscall_invoke*() functions, and the assembly implementation of this routine seems to take r0 as call_id (I'm not familiar with the ABI).

good catch! yes this isn't correct and I think will fail since the call_id is in the wrong position

this prototype seems to be no longer necessary, remove

still think we can ditch this prototype unless I'm not noticing something.

andrewboie · 2017-11-14T22:17:08Z

include/arch/arm/arch.h

 }

 static inline u32_t _arch_syscall_invoke0(u32_t call_id)
 {
-	return 0;
+        return _arm_do_syscall(0, 0, 0, 0, 0, 0, call_id);


One other thing worth considering: system calls are a very hot code path. Here we are spending cycles to marshal 0 values for unused parameters. Are there approaches where unused parameters can just leave undefined values in the registers instead?

If call_id is the first argument (that will always be set), the others don't need to be passed to the function.

i'll see if I can trim this down.

chunlinhan · 2017-11-15T03:26:21Z

arch/arm/core/swap.S

+_escalate_priv:
+    /* check to see if we are already privileged */
+    mrs r2, CONTROL
+    tst r2, #0


Should be:
tst r2, #1
beq _oops

tst r2, #0 the Z flag will always be 1.

Another question is, should we need oops for this situation?

on x86 it is currently not a fatal error to invoke a system call in supervisor mode. it will work its just not a great way to go about using an API.

I don't think we should add any logic or checking to catch this. this is a very hot code path. it will not happen in practice unless the user is doing something really weird.

chunlinhan · 2017-11-15T06:35:35Z

arch/arm/core/thread.c

+	_arm_userspace_enter(user_entry, p1, p2, p3,
+			     (u32_t)_current->stack_obj,
+			     _current->stack_info.size);
+	CODE_UNREACHABLE;


It would be better to add k_thread_abort() before CODE_UNREACHABLE.
Just like what _thread_entry() does to handle the case that thread returns from user_entry()
unexpectedly.

NACK
that isn't how this works, user_entry should never return here

Yeah, I got it after reviewing the x86 implementation.

chunlinhan · 2017-11-15T07:12:45Z

arch/arm/core/userspace.S

+    /* setup arguments to thread_entry function */
+    ldr r0,=_kernel
+    ldr r0,[r0, #_kernel_offset_to_current]
+    bl configure_mpu_stack_guard


I think the stack guard has been configured in __pendsv before _arch_user_mode_enter() being called.

can we just make any adjustments needed to the guard area in C code and not in here?

i also don't have a clear picture on how the guard area, kernel stack, and thread stack are arranged

This gets complicated, because we have to use the guard region differently based on if we are in userspace or not (regardless of if STACK_GUARD is defined). Basically from lowest to highest address the memory is laid out like:
|-----guard------|---------stack-----------------|
with userspace it'll be:
|------guard-----|-------------privileged stack------|-------unprivileged stack--------|

Added complexity is that the mpu regions have to be aligned. So this means when we do userspace we absolutely have to have the stacks aligned to power of 2 boundaries. This means the guard HAS to come out of the stack, not added to the stack. All stacks have to fit in a power of 2 size. The privileged stack also has to be power of 2.

About to push some fixes for this that changes the mpu regions to work properly for user space

andrewboie · 2017-11-15T07:16:38Z

arch/arm/core/userspace.S

+    mov ip, #3
+    msr CONTROL, ip
+
+    /* return to thread_entry */


i don't think this is implemented correctly. r1 appears to contain user_entry when it is supposed to have _thread_entry()'s address in it.

it looks like this instruction will directly enter the user_entry function. What we have to do is setup the stack/registers so that we enter _thread_entry() with user_entry and its three arguments as parameters to that function.
_thread_entry never returns, and calls k_thread_abort() and does other housekeeping if user_entry returns

sorry, looked at wrong spot. this pops r0,r1,r2, and lr. Those are the 3 arguments. And it jumps to the passed in function ptr. Comment is not right. I fixed that.

I think what Andrew mentioned is that the better implementation should be like the following:

_arm_userspace_enter(k_thread_entry_t user_entry,
void *p1, void *p2, void *p3,
u32_t stack_end,
u32_t stack_start)
{
.....
.....
_thread_entry(user_entry, p1, p2, p3);
}

but the current implementation is:

_arm_userspace_enter(k_thread_entry_t user_entry,
void *p1, void *p2, void *p3,
u32_t stack_end,
u32_t stack_start)
{
.....
.....
user_entry(p1, p2, p3);
}

andrewboie · 2017-11-15T07:22:17Z

arch/arm/core/userspace.S

+    push {r0,r1,r2,r3,r4,r5,r6,r7}
+    /* validate syscall limit */
+    ldr ip,=_SYSCALL_LIMIT
+    ldr r0, [sp, #40]


what does this magic value of 40 represent?
can we make this a macro instead?
also this looks very very wrong in that you are validating the system call ID before you elevate to privileged mode, which means the checks aren't useful from a security perspective.
you have to do all checking after the software interrupt is invoked, nothing done when the CPU is in user mode can be trusted

I can move the code to that. That simplifies some things.

andrewboie · 2017-11-15T07:42:24Z

arch/arm/core/swap.S

+    msr CONTROL, r2
+
+    /* fix PSP to point to privileged portion of stack */
+    bx lr


I'm a little confused about how this works..

It seems that upon "svc #3" we land here, and if we are unprivileged, we set mode to privileged, switch to the kernel stack, and return wherever execution was when the "svc #3" call was made.

what's to prevent malicious user code from entering privileged mode anytime they please and then running whatever code they want as a supervisor?

you can't trust the code this returns to...i think what you need to do is re-enable interrupts and handle the system call here in the SVC handler or something like that, which is what we do on x86

We can't do that unfortunately because we cannot handle nested SVC calls. This would mean that we can't context switch without it faulting.

andrewboie · 2017-11-15T08:00:27Z

Since we do not have emulator support, sanitycheck can't be used to verify this.

I suggest running the following test cases to validate the port. CONFIG_USERSPACE=y should be added to the defconfig for whatever platform you are testing on (ideally, one device with an ARM MPU and one with an NXP MPU)

tests/kernel/mem_protect/obj_validation/
tests/kernel/msgq/msgq_api/
tests/kernel/stack/stack_api/
tests/kernel/semaphore/sema_api/
tests/kernel/threads/lifecycle/thread_init/
tests/kernel/threads/custom_data/custom_data_api/
tests/kernel/mutex/mutex/
tests/kernel/alert/alert_api/
tests/kernel/threads/lifecycle/lifecycle_api/

Also test with CONFIG_HW_STACK_PROTECTION enabled and disabled.

I reviewed the code some more, I think I found a showstopper with how the privilege elevation is handled.

chunlinhan · 2017-11-16T07:54:16Z

arch/arm/core/userspace.S

+#endif
+    mov r2,ip
+    sub r2,#24		/* don't erase the pushed regs */
+    bl memset


r0 will be stack_obj + stack_info.size here, so memset() will set the memory out of the range of stack_obj.

has this been fixed?

yeah now we switch over to the priv stack and then memset out the complete user mode stack area. No need to dance around any stored info here.

stephensmalley · 2017-11-17T19:13:47Z

In case you don't already know it, if you try to build tests/kernel/msgq/msgq_api with CONFIG_USERSPACE=y and run it on frdm_k64f, it falls over with:

__usage_fault () at /home/sds/zephyr-project/arch/arm/core/fault_s.S:88
88		eors.n r0, r0
(gdb) where
#0  __usage_fault () at /home/sds/zephyr-project/arch/arm/core/fault_s.S:88
#1  <signal handler called>
#2  0x00001390 in _arch_syscall_invoke2 (call_id=57, arg2=22, arg1=536876204)
    at /home/sds/zephyr-project/include/arch/arm/arch.h:413
#3  k_str_out (n=22, 
    c=0x200014ac <_interrupt_stack+1932> "***** BUS FAULT *****\n")
    at /home/sds/zephyr-project/tests/kernel/msgq/msgq_api/build/frdm_k64f/zephyr/include/generated/syscalls/kernel.h:109
#4  buf_flush (ctx=0x200014a4 <_interrupt_stack+1924>)
    at /home/sds/zephyr-project/misc/printk.c:237
#5  0x00000000 in ?? ()```

stephensmalley · 2017-11-17T19:58:28Z

arch/arm/core/swap.S

+    cmp r1, #3
+    beq _do_syscall
+#endif
+


Would it be possible to check up front if the caller is unpriv and only allow it to use svc 3 in that case? Otherwise, we're allowing userspace to invoke any of the other svc calls, and some of those might be trusting the caller.

Sure, the mode is in the CONTROL reg, and I did this check and bombed out in a previous version of this if the code was privileged. I am on the fence on this one

My concern is different - I don't care about preventing a privileged caller from making an arbitrary svc call; I just want to prevent an unprivileged caller from calling anything other than svc 3 (and any other individual svcs that are explicitly whitelisted as being safe for use by userspace).

Ah ok I see what you mean. This would mean some sort of macro or interface where we define system call numbers and their access mode and then do a lookup to validate it in the actual svc call.

I don't think that's what Stephen is saying.
What we want to prevent is user mode from invoking (for example) SVC #1. That would be a backdoor into the system. On x86 they are different interrupt vectors with different privilege levels, but since this is a common handler on ARM we need to filter this.

Then I think we need to refactor the design.
Right now, we are using SVC for stuff which should never be allowable from user mode: irq_offload() and inducing a kernel panic. irq_offload runs an arbitrary function pointer in interrupt context. kernel panic hoses the entire system (unlike an oops which just kills a thread).
Is there no way to check what the interrupting context's privilege level was?

Yes you know the callers privilege level. My point is that the SVC has always been meant to be called from unprivileged contexts. If we want to add this extra consideration, so be it.

My point is that the SVC has always been meant to be called from unprivileged contexts

For the irq_offload() and panic cases, we are using SVC because we want to generate a CPU exception from software, even though these should not be allowable from user mode.
Is there perhaps a better way to do it?
For sake of efficiency, we could check that the CPU is not in user mode in the logic for panic/offload cases and jump to the oops case if the test fails.

And what do we return if we invoke a syscall from privileged context? We shouldn't allow that either.

And what do we return if we invoke a syscall from privileged context? We shouldn't allow that either.

Do we care?
AFAIK on x86 it does work, although it's a terribly inefficient thing to do since you can just call the implementation directly.
I'm fine with leaving the scenario as "undefined behavior" and not doing checks, or only checking in an assertion. I can't conceive on how supervisor mode would be invoking system calls like this unless someone is doing something really weird, and we don't try to prevent malfeasance in supervisor mode anyway.
I don't think it's worth spending cycles checking for this since it is such a hot code path.

dbkinder

some questions and suggested edits

dbkinder · 2018-02-01T23:27:51Z

doc/kernel/usermode/arm_userspace.rst

+guard (if applicable).
+
+During system calls, the user mode thread's access to the system call and the
+passed in parameters are all validated.  The user mode thread is then elevated


passed-in (with a hyphen)

dbkinder · 2018-02-01T23:30:00Z