unexpected behavior when doing syscall with 7 or more arguments

[wentongwu]

Got some interesting code as below which is running on Cortex-M7 with user space enabled, the tool-chain is Zephyr SDK 0.10.3(somehow it's not easy to update it to latest one) and the code base is 1.14 branch.

The code from 60005bfc to 60005c1e is doing arguments preparation to call api->chnl_read_async((uint32_t)&(((struct adc_driver_api *)0)->chnl_read_async) = 36), but the code located at 60005c08 (strd r2, r1, [sp, #52]) which is trying to put margs->arg6 at [sp, #52] and put margs->arg7 at [sp, #56] will cause the stack (before calling this function) overflow 4 bytes(it may be easily understood with below layout of the privileged stack of the calling thread). So the unprivileged SP will be wrong when back to original function that called SVC.

And I made a patch temporally(if more arguments, need push more. It's not solution) as below to workaround it and it will work as expected. But we need find out why compiler behaves like this, currently IMHO it's most probably a bug of compiler, welcome any idea. Maybe we also need add some test cases to cover more than 6 arguments syscall.

 * |        ...          |
 * |        ...          |
 * |---------------------- <---- Stack Ptr when PC at 0x60005bcc (sp = Stack Ptr before calling z_hdlr_adc_chnl_read_async - 48)
 * |        ...          |
 * |        ...          |
 * |        ...          |
 * |---------------------- <---- Stack Ptr before calling z_hdlr_adc_chnl_read_async
 * |  r4                 |
 * |----------------------- 
 * |  r5                 |
 * |----------------------- 
 * |  unprivileged SP    |
 * |----------------------- 
 * |  LR                 |
 * |----------------------- 
 * |        ...          |
 * |        ...          |

--- a/arch/arm/core/userspace.S
+++ b/arch/arm/core/userspace.S
@@ -233,7 +233,7 @@ SECTION_FUNC(TEXT, z_arm_do_syscall)
 
 valid_syscall:
     /* push args to complete stack frame */
-    push {r4,r5}
+    push {r4,r5,r6}
 
 dispatch_syscall:
     ldr ip, =_k_syscall_table
@@ -244,7 +244,7 @@ dispatch_syscall:
     blx ip
 
     /* restore LR */
-    ldr lr, [sp,#12]
+    ldr lr, [sp, #16]

 #if defined(CONFIG_BUILTIN_STACK_GUARD)
     /* clear stack limit (stack protection not required in user mode) */
@@ -253,7 +253,7 @@ dispatch_syscall:
 #endif
 
     /* set stack back to unprivileged stack */
-    ldr ip, [sp,#8]
+    ldr ip, [sp, #12]
     msr PSP, ip
 
     push {r0, r1}

Z_SYSCALL_HANDLER(adc_chnl_read_async, dev, channel_id, buffer, usr_buff_size,
                  ts_buff, more_args_ptr)
{
        volatile struct _syscall_7_args *margs =
                (struct _syscall_7_args  *)more_args_ptr;

        Z_OOPS(Z_SYSCALL_MEMORY_READ(margs, sizeof(*margs)));
        Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, chnl_read_async));
        Z_OOPS(Z_SYSCALL_MEMORY_WRITE(buffer, usr_buff_size));
        Z_OOPS(Z_SYSCALL_OBJ_INIT(margs->arg7, K_OBJ_POLL_SIGNAL));

        return z_impl_adc_chnl_read_async((struct device *)dev,
                            (u8_t) channel_id, (u16_t *)buffer,
                            (size_t) usr_buff_size, (u64_t *)ts_buff,
                             margs->arg6, margs->arg7);
}

static inline int z_impl_adc_chnl_read_async(struct device *dev,
                                        const u8_t channel_id, u16_t *buffer,
                                        size_t usr_buff_size, u64_t *ts_buff,
                                        const u32_t sample_count,
                                        struct k_poll_signal *async)
{
        const struct adc_driver_api *api = dev->driver_api;

        return api->chnl_read_async(dev, channel_id, buffer, usr_buff_size,
                                    ts_buff, sample_count, async);
}

Z_SYSCALL_HANDLER(adc_chnl_read_async, dev, channel_id, buffer, usr_buff_size,
                  ts_buff, more_args_ptr)
{
60005bc8:       e92d 4ff7       stmdb   sp!, {r0, r1, r2, r4, r5, r6, r7, r8, r9, sl, fp, lr}
60005bcc:       e9dd b40c       ldrd    fp, r4, [sp, #48]       ; 0x30
60005bd0:       4605            mov     r5, r0
60005bd2:       468a            mov     sl, r1
60005bd4:       4617            mov     r7, r2
        volatile struct _syscall_7_args *margs =
                (struct _syscall_7_args  *)more_args_ptr;

        Z_OOPS(Z_SYSCALL_MEMORY_READ(margs, sizeof(*margs)));
60005bd6:       2108            movs    r1, #8
60005bd8:       2200            movs    r2, #0
60005bda:       4620            mov     r0, r4
{
60005bdc:       4698            mov     r8, r3
60005bde:       9e0e            ldr     r6, [sp, #56]   ; 0x38
        Z_OOPS(Z_SYSCALL_MEMORY_READ(margs, sizeof(*margs)));
60005be0:       f009 f92b       bl      6000ee3a <z_arch_buffer_validate>
60005be4:       4681            mov     r9, r0
60005be6:       2800            cmp     r0, #0
60005be8:       d035            beq.n   60005c56 <z_hdlr_adc_chnl_read_async+0x8e>
60005bea:       4b26            ldr     r3, [pc, #152]  ; (60005c84 <z_hdlr_adc_chnl_read_async+0xbc>)
60005bec:       4622            mov     r2, r4
60005bee:       9300            str     r3, [sp, #0]
60005bf0:       2308            movs    r3, #8
        Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, chnl_read_async));
        Z_OOPS(Z_SYSCALL_MEMORY_WRITE(buffer, usr_buff_size));
60005bf2:       4925            ldr     r1, [pc, #148]  ; (60005c88 <z_hdlr_adc_chnl_read_async+0xc0>)
60005bf4:       4825            ldr     r0, [pc, #148]  ; (60005c8c <z_hdlr_adc_chnl_read_async+0xc4>)
60005bf6:       f7fb fc17       bl      60001428 <printk>
60005bfa:       e03f            b.n     60005c7c <z_hdlr_adc_chnl_read_async+0xb4>
        Z_OOPS(Z_SYSCALL_OBJ_INIT(margs->arg7, K_OBJ_POLL_SIGNAL));

        return z_impl_adc_chnl_read_async((struct device *)dev,
60005bfc:       6822            ldr     r2, [r4, #0]
        return api->chnl_read_async(dev, channel_id, buffer, usr_buff_size,
60005bfe:       4628            mov     r0, r5
                            (u8_t) channel_id, (u16_t *)buffer,
                            (size_t) usr_buff_size, (u64_t *)ts_buff,
                             margs->arg6, margs->arg7);
60005c00:       6861            ldr     r1, [r4, #4]
60005c02:       686b            ldr     r3, [r5, #4]
60005c04:       f8cd b030       str.w   fp, [sp, #48]   ; 0x30
60005c08:       e9cd 210d       strd    r2, r1, [sp, #52]       ; 0x34
60005c0c:       6a5c            ldr     r4, [r3, #36]   ; 0x24
60005c0e:       463a            mov     r2, r7
60005c10:       4643            mov     r3, r8
60005c12:       fa5f f18a       uxtb.w  r1, sl
60005c16:       46a4            mov     ip, r4
}
60005c18:       b003            add     sp, #12
60005c1a:       e8bd 4ff0       ldmia.w sp!, {r4, r5, r6, r7, r8, r9, sl, fp, lr}
60005c1e:       4760            bx      ip
        Z_OOPS(Z_SYSCALL_OBJ_INIT(margs->arg7, K_OBJ_POLL_SIGNAL));
60005c20:       6860            ldr     r0, [r4, #4]
60005c22:       f7fa fb61       bl      600002e8 <z_object_find>
60005c26:       6861            ldr     r1, [r4, #4]
60005c28:       2301            movs    r3, #1
60005c2a:       2206            movs    r2, #6
60005c2c:       f009 f91c       bl      6000ee68 <z_obj_validation_check>
60005c30:       2800            cmp     r0, #0
60005c32:       d0e3            beq.n   60005bfc <z_hdlr_adc_chnl_read_async+0x34>
60005c34:       4914            ldr     r1, [pc, #80]   ; (60005c88 <z_hdlr_adc_chnl_read_async+0xc0>)
60005c36:       4816            ldr     r0, [pc, #88]   ; (60005c90 <z_hdlr_adc_chnl_read_async+0xc8>)
60005c38:       f7fb fbf6       bl      60001428 <printk>
60005c3c:       e01e            b.n     60005c7c <z_hdlr_adc_chnl_read_async+0xb4>
        Z_OOPS(Z_SYSCALL_MEMORY_WRITE(buffer, usr_buff_size));
60005c3e:       2201            movs    r2, #1
60005c40:       4641            mov     r1, r8
60005c42:       4638            mov     r0, r7
60005c44:       f009 f8f9       bl      6000ee3a <z_arch_buffer_validate>
60005c48:       2800            cmp     r0, #0
60005c4a:       d0e9            beq.n   60005c20 <z_hdlr_adc_chnl_read_async+0x58>
60005c4c:       4b11            ldr     r3, [pc, #68]   ; (60005c94 <z_hdlr_adc_chnl_read_async+0xcc>)
60005c4e:       463a            mov     r2, r7
60005c50:       9300            str     r3, [sp, #0]
60005c52:       4643            mov     r3, r8
60005c54:       e7cd            b.n     60005bf2 <z_hdlr_adc_chnl_read_async+0x2a>
        Z_OOPS(Z_SYSCALL_DRIVER_ADC(dev, chnl_read_async));
60005c56:       4628            mov     r0, r5
60005c58:       f7fa fb46       bl      600002e8 <z_object_find>
60005c5c:       464b            mov     r3, r9
60005c5e:       220d            movs    r2, #13
60005c60:       4629            mov     r1, r5
60005c62:       f009 f901       bl      6000ee68 <z_obj_validation_check>
60005c66:       2800            cmp     r0, #0
60005c68:       d1e4            bne.n   60005c34 <z_hdlr_adc_chnl_read_async+0x6c>
60005c6a:       686b            ldr     r3, [r5, #4]
60005c6c:       6a5a            ldr     r2, [r3, #36]   ; 0x24
60005c6e:       2a00            cmp     r2, #0
60005c70:       d1e5            bne.n   60005c3e <z_hdlr_adc_chnl_read_async+0x76>
60005c72:       4a09            ldr     r2, [pc, #36]   ; (60005c98 <z_hdlr_adc_chnl_read_async+0xd0>)
60005c74:       4904            ldr     r1, [pc, #16]   ; (60005c88 <z_hdlr_adc_chnl_read_async+0xc0>)
60005c76:       4809            ldr     r0, [pc, #36]   ; (60005c9c <z_hdlr_adc_chnl_read_async+0xd4>)
60005c78:       f7fb fbd6       bl      60001428 <printk>
        Z_OOPS(Z_SYSCALL_OBJ_INIT(margs->arg7, K_OBJ_POLL_SIGNAL));
60005c7c:       4630            mov     r0, r6
60005c7e:       f7ff f933       bl      60004ee8 <z_arch_syscall_oops>
60005c82:       bf00            nop
60005c84:       60011b3d        .word   0x60011b3d
60005c88:       60011aae        .word   0x60011aae
60005c8c:       6001116e        .word   0x6001116e
60005c90:       6001139c        .word   0x6001139c
60005c94:       600111c9        .word   0x600111c9
60005c98:       60011b42        .word   0x60011b42
60005c9c:       600113c4        .word   0x600113c4

[wentongwu]

@andrewboie @ioannisg

[wentongwu]

@stephanosio @andyross

[andrewboie]

My general experience is that every time I've suspected a compiler bug, I've been wrong, except once.
Have you looked over the definitions of arch_syscall implementations for ARM?

[wentongwu]

Have you looked over the definitions of arch_syscall implementations for ARM?

@andrewboie sure, z_arm_do_syscall is in the path.

My general experience is that every time I've suspected a compiler bug, I've been wrong, except once.

maybe I'm wrong, but compiler puts the 7th argument on the wrong place...

[wentongwu]

diff --git a/tests/kernel/mem_protect/syscalls/src/main.c b/tests/kernel/mem_protect/syscalls/src/main.c
index ddf6a38463..b86276d519 100644
--- a/tests/kernel/mem_protect/syscalls/src/main.c
+++ b/tests/kernel/mem_protect/syscalls/src/main.c
@@ -201,6 +201,37 @@ void test_to_copy(void)

 K_MEM_POOL_DEFINE(test_pool, BUF_SIZE, BUF_SIZE, 4, 4);

+uint32_t z_impl_sum_arg7(uint32_t arg1, uint32_t arg2, uint32_t arg3,
+                        uint32_t arg4, uint32_t arg5, uint32_t arg6,
+                        uint32_t arg7)
+{
+       uint32_t args[] = { arg1, arg2, arg3, arg4, arg5, arg6, arg7 };
+       uint32_t ret = 0xaef464;
+
+       for (int i = 0; i < ARRAY_SIZE(args); i++) {
+               ret += args[i];
+               ret = (ret << 11) | (ret >> 23);
+       }
+
+       return ret;
+}
+
+Z_SYSCALL_HANDLER(sum_arg7, arg1, arg2, arg3, arg4, arg5, more_args_ptr)
+{
+       volatile struct _syscall_7_args *margs =
+               (struct _syscall_7_args *)more_args_ptr;
+
+       return z_impl_sum_arg7(arg1, arg2, arg3, arg4, arg5,
+                              margs->arg6, margs->arg7);
+}
+
+void test_arg7(void)
+{
+       uint32_t ret = sum_arg7(1, 2, 3, 4, 5, 6, 7);
+
+       printk("the sum is %d\n", ret);
+}
+
 void test_main(void)
 {
        sprintf(kernel_string, "this is a kernel string");
@@ -211,6 +242,7 @@ void test_main(void)
                         ztest_unit_test(test_string_nlen),
                         ztest_user_unit_test(test_string_nlen),
                         ztest_user_unit_test(test_to_copy),
+                        ztest_user_unit_test(test_arg7),
                         ztest_user_unit_test(test_user_string_copy),
                         ztest_user_unit_test(test_user_string_alloc_copy));
        ztest_run_test_suite(syscalls);
diff --git a/tests/kernel/mem_protect/syscalls/src/test_syscalls.h b/tests/kernel/mem_protect/syscalls/src/test_syscalls.h
index 9c0645b008..81991406ad 100644
--- a/tests/kernel/mem_protect/syscalls/src/test_syscalls.h
+++ b/tests/kernel/mem_protect/syscalls/src/test_syscalls.h
@@ -16,6 +16,10 @@ __syscall int to_copy(char *dest);

 __syscall size_t string_nlen(char *src, size_t maxlen, int *err);

+__syscall uint32_t sum_arg7(uint32_t arg1, uint32_t arg2, uint32_t arg3,
+                           uint32_t arg4, uint32_t arg5, uint32_t arg6,
+                           uint32_t arg7);
+
 #include <syscalls/test_syscalls.h>

 #endif /* _TEST_SYSCALLS_H_ */

above code on 1.14 branch with 0.10.3 SDK will also cause mpu fault. checking...

[wentongwu]

same error on mps2_an385 both with SDK 0.10.3 and SDK 0.11.4

00006f4a <z_hdlr_sum_arg7>:
{
    6f4a:       b430            push    {r4, r5}
    6f4c:       9c03            ldr     r4, [sp, #12]
        return z_impl_sum_arg7(arg1, arg2, arg3, arg4, arg5,
    6f4e:       6825            ldr     r5, [r4, #0]
    6f50:       6864            ldr     r4, [r4, #4]
    6f52:       e9cd 5403       strd    r5, r4, [sp, #12]
}
    6f56:       bc30            pop     {r4, r5}
        return z_impl_sum_arg7(arg1, arg2, arg3, arg4, arg5,
    6f58:       f7f9 bd26       b.w     9a8 <z_impl_sum_arg7>

but qemu_x86 works well with SDK 0.10.3

[wentongwu]

diff --git a/arch/arm/core/userspace.S b/arch/arm/core/userspace.S
index 60888c08cb..6c492cae9d 100644
--- a/arch/arm/core/userspace.S
+++ b/arch/arm/core/userspace.S
@@ -233,7 +233,8 @@ SECTION_FUNC(TEXT, z_arm_do_syscall)

 valid_syscall:
     /* push args to complete stack frame */
-    push {r4,r5}
+    mov ip, sp
+    push {r4,r5,ip}

 dispatch_syscall:
     ldr ip, =_k_syscall_table
@@ -244,7 +245,7 @@ dispatch_syscall:
     blx ip

     /* restore LR */
-    ldr lr, [sp,#12]
+    ldr lr, [sp,#16]

 #if defined(CONFIG_BUILTIN_STACK_GUARD)
     /* clear stack limit (stack protection not required in user mode) */
@@ -253,7 +254,7 @@ dispatch_syscall:
 #endif

     /* set stack back to unprivileged stack */
-    ldr ip, [sp,#8]
+    ldr ip, [sp,#12]
     msr PSP, ip

     push {r0, r1}

the root cause is that we don't push ssf to the stack, and almost all of the handlers don't use it, so compiler replace the memory of ssf as above assembly code behaves.
@ioannisg can you please take a look this patch, if no comments, I will submit the PR. Thanks

[andrewboie]

Nice work, thanks @wentongwu
Can we create a test for this?

[wentongwu]

Nice work, thanks @wentongwu
Can we create a test for this?

@andrewboie sure, I will.