arch: arm: switch to privilege stack in SVC handler #95101

wearyzen · 2025-08-28T09:11:40Z

Initialize the privilege stack and switch PSP to it early in the SVC handler to ensure z_arm_do_syscall does not start on a user-accessible stack frame. Also add a test to validate this behavior.

Introduce a test to overwrite the return address in the exception stack frame of a lower-priority thread performing an SVC call. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

wearyzen · 2025-09-04T09:25:30Z

Hi @andyross could you please help with the review of this PR?

ceolin

It looks right to me.

wearyzen · 2025-09-08T14:59:01Z

Hi @andyross, @bbolen, @carlocaione, @galak, @MaureenHelm, @microbuilder, @stephanosio
could you please help with the review of this PR?
Thanks!

wearyzen · 2025-09-08T15:00:51Z

Also, requesting @mathieuchopstm for a review since he has been involved with Arm assembly reviews for a while

mathieuchopstm

I'll give it a second pass tomorrow. Overall seems OK but I may have missed some details (haven't looked at z_arm_do_syscall either so some things still seem a bit weird to me)

arch/arm/core/cortex_m/swap_helper.S

`west build` does not build the `empty_cpu0` application making `west build -b mps2/an521/cpu1 -t run` fail because of the missing empty_cpu0's binary. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

wearyzen · 2025-09-11T09:18:53Z

Hi @mathieuchopstm could you please have another look at this?

mathieuchopstm

Some comments apply to the ARMv7/8-Mainline as well but I did not write them twice.

Additionally, ~L305 of userspace.S says:

 * This function is used to do system calls from unprivileged code. This * function is responsible for the following: * 1) Fixing up bad syscalls * 2) Configuring privileged stack and loading up stack arguments * 3) Dispatching the system call * 4) Restoring stack and calling back to the caller of the SVC

This should be updated to remove (2) since it's now done in SVC handler.

The comment right under also says

 * Note [when using built-in stack limit checking on ARMv8-M]: * At this point PSPLIM is already configured to guard the default (user) * stack, so pushing to the default thread's stack is safe.

That also needs to be updated.

arch/arm/core/cortex_m/swap_helper.S

Initialize the privilege stack and switch PSP to it early in the SVC handler to ensure `z_arm_do_syscall` does not start on a user-accessible stack frame. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

sonarqubecloud · 2025-09-11T20:24:09Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

wearyzen · 2025-09-12T11:17:13Z

@mathieuchopstm I have tried to update the table with more comments, let me know if it looks good.
Also, note that there are some optimizations/redundancies identified in z_arm_do_syscall (e.g. currently it requires return address in both sp[4] and r8) but that will be handled as a follow up PR. This PR is intended only to fix the behavior tested in the 1st commit.
Thanks!

mathieuchopstm

Minor non-blockers. LGTM otherwise

mathieuchopstm · 2025-09-12T12:13:52Z

arch/arm/core/userspace.S

 * located at *lower* memory area. Since we switch to the top of the
 * privileged stack we are safe, as long as the stack can accommodate
 * the maximum exception stack frame.
 */


I guess this whole comment can be removed too since we pivot PSPLIM while in Handler mode?

Suggested change

*/

mathieuchopstm · 2025-09-12T12:16:25Z

arch/arm/core/cortex_m/swap_helper.S

+ beq .L_no_padding
+ /* special handling for padded sf */
+ bics r1, r3 /* clear the pad bit (priv stack is aligned and doesn't need it) */
+ adds r2, #4


I'd really comment this here, it's not obvious that this is the +4B comment right below talks about

Suggested change

adds r2, #4

adds r2, #4 /* add +4B to compensate padding (see below) */

mathieuchopstm · 2025-09-12T12:22:26Z

arch/arm/core/cortex_m/swap_helper.S

+ /* Calculate original pre-svc user sp with pad */
+ add r2, #4


Single line would be sufficient + reword proposal:

Suggested change

/* Calculate original pre-svc user sp with pad */

add r2, #4

add r2, #4 /* take pad into account for pre-svc user sp computation */

TaiJuWu · 2025-09-29T02:08:37Z

tests/arch/arm/arm_user_stack_test/src/main.c

+{
+printf("Call %s from %s\n", __func__, k_is_user_context() ? "user" : "kernel");
+while (1) {
+attack_sp[-2] = (int)attack_entry;


Could you explain why the index is -2 instead of -3, -1 or 0?
I meet an issue related this, thanks!

The following patch cause this test fail but I have no idea about this issue.

diff --git a/kernel/sched.c b/kernel/sched.c index 37fdbf3bf0b..b119381ef83 100644 --- a/kernel/sched.c +++ b/kernel/sched.c @@ -77,6 +77,7 @@ static ALWAYS_INLINE void *curr_cpu_runq(void) static ALWAYS_INLINE void runq_add(struct k_thread *thread) { __ASSERT_NO_MSG(!z_is_idle_thread_object(thread)); + __ASSERT_NO_MSG(!is_thread_dummy(thread)); _priq_run_add(thread_runq(thread), thread); }

attack_sp is the value of sp upon execution of the svc instruction. The hardware then saves processor context on this stack as follows:

Notice that ReturnAddress - the address at which execution should resume upon return from exception - is at offset sp - 0x20 + 0x18 = sp - 8, a.k.a. attack_sp[-2] since attack_sp is a uint32_t* here

attack_sp is the value of sp upon execution of the svc instruction. The hardware then saves processor context on this stack as follows:

Notice that ReturnAddress - the address at which execution should resume upon return from exception - is at offset sp - 0x20 + 0x18 = sp - 8, a.k.a. attack_sp[-2] since attack_sp is a uint32_t* here

Got it, thanks for your detailed explanation.

wearyzen requested review from andyross and ceolin August 28, 2025 09:12

zephyrbot added area: ARM ARM (32-bit) Architecture platform: ARM Arm Limited labels Aug 28, 2025

zephyrbot requested review from MaureenHelm, bbolen, carlocaione, galak, ithinuel, microbuilder and stephanosio August 28, 2025 09:12

zephyrbot assigned wearyzen Aug 28, 2025

tests: overwrite return address of stack frame

3fb7387

Introduce a test to overwrite the return address in the exception stack frame of a lower-priority thread performing an SVC call. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

wearyzen force-pushed the change_syscall_stack branch from 8a235e3 to db4c1d7 Compare August 28, 2025 09:29

ceolin previously approved these changes Sep 5, 2025

View reviewed changes

wearyzen requested a review from mathieuchopstm September 8, 2025 14:59

mathieuchopstm reviewed Sep 8, 2025

View reviewed changes

boards: arm: mps2: Add dependency to empty_cpu0 for an521/cpu1's app

a4eef4c

`west build` does not build the `empty_cpu0` application making `west build -b mps2/an521/cpu1 -t run` fail because of the missing empty_cpu0's binary. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

wearyzen dismissed ceolin’s stale review via 3b71022 September 9, 2025 16:46

wearyzen force-pushed the change_syscall_stack branch from db4c1d7 to 3b71022 Compare September 9, 2025 16:46

zephyrbot added the area: Architectures label Sep 9, 2025

zephyrbot requested review from dcpleung and nashif September 9, 2025 16:47

wearyzen force-pushed the change_syscall_stack branch 2 times, most recently from a933d24 to 86c8de7 Compare September 9, 2025 17:31

wearyzen requested a review from mathieuchopstm September 9, 2025 18:21

mathieuchopstm reviewed Sep 11, 2025

View reviewed changes

arch: arm: switch to privilege stack in SVC handler

df57f95

Initialize the privilege stack and switch PSP to it early in the SVC handler to ensure `z_arm_do_syscall` does not start on a user-accessible stack frame. Signed-off-by: Sudan Landge <sudan.landge@arm.com>

wearyzen force-pushed the change_syscall_stack branch from 86c8de7 to df57f95 Compare September 11, 2025 19:58

mathieuchopstm approved these changes Sep 12, 2025

View reviewed changes

wearyzen requested a review from ceolin September 12, 2025 14:13

nashif approved these changes Sep 13, 2025

View reviewed changes

nashif merged commit 319c697 into zephyrproject-rtos:main Sep 13, 2025
34 checks passed

wearyzen added backport v3.7-branch Request backport to the v3.7-branch backport v4.1-branch Request backport to the v4.1-branch backport v4.2-branch Request backport to the v4.2-branch labels Sep 15, 2025

This was referenced Sep 15, 2025

[Backport v3.7-branch] Failed to backport #95101 #96013

Closed

[Backport v4.2-branch] arch: arm: switch to privilege stack in SVC handler #96014

Merged

[Backport v4.1-branch] arch: arm: switch to privilege stack in SVC handler #96015

Merged

JarmouniA mentioned this pull request Sep 15, 2025

[Backport v3.7-branch] arch: arm: switch to privilege stack in SVC handler #96030

Merged

wearyzen mentioned this pull request Sep 17, 2025

Backport #95101 (arch: arm: switch to privilege stack in SVC handler) #96172

Closed

1 task

TaiJuWu reviewed Sep 29, 2025

View reviewed changes

	adds r2, #4
	adds r2, #4 /* add +4B to compensate padding (see below) */

	/* Calculate original pre-svc user sp with pad */
	add r2, #4
	add r2, #4 /* take pad into account for pre-svc user sp computation */

arch: arm: switch to privilege stack in SVC handler #95101

arch: arm: switch to privilege stack in SVC handler #95101

Uh oh!

Conversation

wearyzen commented Aug 28, 2025

wearyzen commented Sep 4, 2025

ceolin left a comment

Choose a reason for hiding this comment

wearyzen commented Sep 8, 2025

wearyzen commented Sep 8, 2025

mathieuchopstm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wearyzen commented Sep 11, 2025

mathieuchopstm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Sep 11, 2025

Quality Gate passed

wearyzen commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mathieuchopstm left a comment

Choose a reason for hiding this comment

mathieuchopstm Sep 12, 2025

Choose a reason for hiding this comment

mathieuchopstm Sep 12, 2025

Choose a reason for hiding this comment

mathieuchopstm Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

TaiJuWu Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mathieuchopstm Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

TaiJuWu Sep 29, 2025

Choose a reason for hiding this comment

Labels

6 participants

wearyzen commented Sep 12, 2025 •

edited

Loading

TaiJuWu Sep 29, 2025 •

edited

Loading

mathieuchopstm Sep 29, 2025 •

edited

Loading