mpu stack guard: fix stack alignement #500

jamike · 2017-06-15T04:09:24Z

-1- The mpu stack guard feature requires that stack base is 32 bytes aligned.
(ARM DUI0553.pdf : Cortex M4 generic User guide mentions this constraint)
-2- Mpu stack guard test does not use kthread_spawn api, because it is not
useable with mpu stack guard: this api allocates the kthread structure at
stack base address, and mpu stack guard does not allow any access
in the 32 bytes after stack base address.
-3- Mpu stack guard test has been modified to be able to detect above issue.

MaureenHelm · 2017-06-15T18:13:27Z

samples/mpu_stack_guard_test/src/main.c

@@ -65,6 +66,5 @@ void main(void)
 	printk("Canary Initial Value = 0x%x\n", buf.canary);

 	/* spawn stack_guard_thread */
-	k_thread_spawn(buf.stack, STACKSIZE, stack_guard_thread, NULL, NULL,
-		       NULL, PRIORITY, 0, K_NO_WAIT);
+	k_thread_create(&buf.thread, buf.stack, STACKSIZE, stack_guard_thread,			NULL,NULL, NULL, PRIORITY, 0, K_NO_WAIT);


This was recently fixed in #478, but the arm branch hasn't been rebased yet to pick it up.

if you rebase, we should pickup the fix now in the arm branch

MaureenHelm · 2017-06-15T18:18:50Z

samples/mpu_stack_guard_test/prj_stack_guard.conf

@@ -1,5 +1,4 @@
 CONFIG_STDOUT_CONSOLE=y
 CONFIG_MPU_STACK_GUARD=y
 CONFIG_SYS_LOG=y
-CONFIG_SYS_LOG_DEFAULT_LEVEL=4
-CONFIG_SYS_LOG_OVERRIDE_LEVEL=4
+CONFIG_MULTITHREADING=y


You don't need to enable multithreading explicitly, it's enabled by default

MaureenHelm · 2017-06-15T18:46:58Z

samples/mpu_stack_guard_test/src/main.c

-		printk("Canary = 0x%08x\tTest not passed.\n", buf.canary);
+
+	if (buf[num].canary != STACK_GUARD_CANARY) {
+		printk("Canary = 0x%08x\tTest not passed.\n", buf[num].canary);


I tried running this on frdm_k64f without stack guarding enabled and I don't hit this condition anymore.

Your point is reproduced on nucleo_f401re.
If I moved struct k_thread thread before canary, the message test not passed occurs on nucleo_f401re (because canary gets corrupted before kthread structure get corrupted by stack overflow)
In this case , log is as follow:

STACK_ALIGN 4
MPU STACK GUARD Test
MPU STACK GUARD Test
MPU STACK GUARD Test
MPU STACK GUARD Test
Canary Initial Value = 0xf0cacc1a threads 0x20000b9c
Canary = 0xfffffff5 Test not passed.
***** MPU FAULT *****
Executing thread ID (thread): 0x20000b9c
Faulting instruction address: 0x80025b0
Data Access Violation
Address: 0x8000edb
Fatal fault in thread 0x20000b9c! Aborting.
***** HARD FAULT *****
Fault escalation (see below)
***** MPU FAULT *****
Executing thread ID (thread): 0x20000b9c
Faulting instruction address: 0x8001db6
Data Access Violation
Address: 0x8000edb
Fatal fault in ISR! Spinning...

On the other hand , the log present in README.rst is not describing a mpu guard working usecase: the instruction causing the initial MPU FAULT is at address 0x0, it should be a valid address showing that the MPU caught the instruction that has overflowed the stack.
=>i ll will update README.rst in next PR update

galak · 2017-06-15T20:11:45Z

I've updated the arm branch to be in sync with master to pull in the fix @MaureenHelm mentioned.

jamike · 2017-06-16T19:05:53Z

Comment taken into account and patch rebased.

dbkinder · 2017-06-19T18:38:06Z

samples/mpu_stack_guard_test/README.rst

+      Faulting instruction address:  0x8001db6
+      Data Access Violation
+      Address: 0x8000edb
+    Fatal fault in ISR! Spinning...

 With the MPU enabled and the stack guard feature present:


might change "present" to "enabled" to parallel the previous heading

dbkinder

one wording suggestion, but otherwise OK.

andrewboie · 2017-06-20T16:41:45Z

You should consider implementing ARM versions of the stack definition macros, K_THREAD_STACK_DEFINE, etc. You can then reserve an additional 32-bytes at the beginning of the stack buffers for the guard area so that the effective stack size is the same as the user requested.

Signed-off-by: Kumar Gala <[email protected]>

We define a variable to pickup a default for the bossa binary, however we weren't using it. Lets do so now. Signed-off-by: Kumar Gala <[email protected]>

If we have more than one board of a given type we need to be able to specify the board_id to select which specific board that the pyocd command should target. Introduce an environment variable PYOCD_BOARD_ID to we can set that will get passed to the pyocd command that needs it. Here's an example: $ make -C samples/hello_world/ BOARD=frdm_k64f flash PYOCD_BOARD_ID=1234 Signed-off-by: Kumar Gala <[email protected]>

As there are multiple ways to flash or debug (pyOCD or openOCD) allow the user to override the default. Signed-off-by: Kumar Gala <[email protected]>

Allow to use an external debug adapter such as J-Link or ULINK connected to a 20-pin JTAG header to flash the image. SWD is the actual protocol used by the debug interface. Signed-off-by: Piotr Mienkowski <[email protected]>

As other stm32 series support MPU, move common file in a file tree useable by socs from other series JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

Since all l4 socs support MPU, add MPU capability in KConfig.series JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

Signed-off-by: Michel Jaouen <[email protected]>

Since not all socs from f3 series (i.e stm32f334x8 no MPU) have MPU capability, add capability only for MPU capable socs in Kconfig.soc JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

Add configuration and memory definitions to support STM32F103x8 Medium-density performance line SoC with 64 KB Flash. Merge multiple files into single Kconfig.defconfig.stm32f103xx Signed-off-by: Siddharth Chandrasekaran <[email protected]>

Following test changes, log info have changes and previous log info with stack guard present shows an mpu error not caused by a stack overflow caught by mpu stack guard feature. Signed-off-by: Michel Jaouen <[email protected]>

jamike · 2017-06-28T07:24:36Z

Are the changes fine for everybody?

galak · 2017-06-28T14:53:05Z

@MaureenHelm you good?

MaureenHelm

Your point is reproduced on nucleo_f401re.
If I moved struct k_thread thread before canary, the message test not passed occurs on nucleo_f401re (because canary gets corrupted before kthread structure get corrupted by stack overflow)

Thanks, this fixes the frdm_k64f as well.

MaureenHelm · 2017-06-29T16:00:52Z

samples/mpu/mpu_stack_guard_test/src/main.c

-	printk("MPU STACK GUARD Test\n");
-	printk("Canary Initial Value = 0x%x\n", buf.canary);
+		/* create stack_guard_thread */
+		k_thread_create(&buf[i].thread, buf[i].stack, STACKSIZE,


I think you need to use K_THREAD_STACK_SIZEOF instead of STACKSIZE

yeah. That adjusts for the alignment adjustment. Otherwise you might get unlucky and bad things will happen. Good catch.

MaureenHelm · 2017-06-29T16:03:38Z

samples/mpu/mpu_stack_guard_test/src/main.c

+	int i;
+	bool failed;
+
+	printk("STACK_ALIGN %x\n", STACK_ALIGN);


Please prepend the value with 0x

agross-oss · 2017-07-01T05:28:36Z

include/arch/arm/arch.h

@@ -42,9 +42,98 @@ extern "C" {
 #include <arch/arm/cortex_m/sys_io.h>
 #include <arch/arm/cortex_m/nmi.h>
 #endif
+#if defined(CONFIG_MPU_STACK_GUARD)
+


Sorry for the late comment, but I found out that we need this regardless of whether or not the STACK_GUARD is set. The reason being that we need these in place to place the stacks in a specific place. Instead of noinit, we can set the location to a specific region during link.

How about just putting the STACK_SIZE in the #if def and set it to 32 or 4 based on the STACK_GUARD being set. The rest would always be defined.

Stacks can really live anywhere that isn't in application memory space.
For stacks that are not defined as struct members, on x86 I put them in a special '.stacks' section which ends up in the kernel's "noinit".

andrewboie

The definition of the various K_THREAD_STACK_xxxx macros looks correct to me for the guard-type stack protection.

agross-oss · 2017-08-02T20:23:44Z

I reworked these patches and pushed to a new pull request. #987

galak · 2017-08-09T18:46:34Z

Merged #987, we can close this now.

jamike force-pushed the fix_arm_mpu_guard_stack_alignement branch 2 times, most recently from 6e262be to 233d892 Compare June 15, 2017 11:31

galak self-assigned this Jun 15, 2017

galak requested review from fvincenzo, andyross and MaureenHelm June 15, 2017 13:21

MaureenHelm requested changes Jun 15, 2017

View reviewed changes

galak force-pushed the arm branch from 6e62f7b to 8176617 Compare June 15, 2017 20:10

galak force-pushed the arm branch from 8176617 to 49326ad Compare June 15, 2017 21:15

galak requested a review from agross-oss June 16, 2017 13:14

jamike force-pushed the fix_arm_mpu_guard_stack_alignement branch from 233d892 to 13d8c7c Compare June 16, 2017 19:02

galak force-pushed the arm branch from bdc838a to 1277f8f Compare June 16, 2017 19:27

dbkinder reviewed Jun 19, 2017

View reviewed changes

dbkinder approved these changes Jun 19, 2017

View reviewed changes

galak force-pushed the arm branch from 7d61052 to dc38879 Compare June 20, 2017 15:52

galak force-pushed the arm branch from c52d348 to 41c85d8 Compare June 22, 2017 14:55

galak and others added 11 commits June 23, 2017 14:37

boards: 96b_nitrogen: Add support for flash/debug with pyOCD

43c0aa9

Signed-off-by: Kumar Gala <[email protected]>

scripts: bossa-flash.sh: fix variable usage

8ad631a

We define a variable to pickup a default for the bossa binary, however we weren't using it. Lets do so now. Signed-off-by: Kumar Gala <[email protected]>

board: frdm_k64f: allow overriding default debug/flash scripts

9f9f210

As there are multiple ways to flash or debug (pyOCD or openOCD) allow the user to override the default. Signed-off-by: Kumar Gala <[email protected]>

boards: sam_e70_xplained: allow flashing via JTAG header

7a78fe8

Allow to use an external debug adapter such as J-Link or ULINK connected to a 20-pin JTAG header to flash the image. SWD is the actual protocol used by the debug interface. Signed-off-by: Piotr Mienkowski <[email protected]>

arm: soc: stm32: make mpu f4 config useable for other family

fb10290

As other stm32 series support MPU, move common file in a file tree useable by socs from other series JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

arm: soc: stm32: l4: add MPU capability for series

01c0d83

Since all l4 socs support MPU, add MPU capability in KConfig.series JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

boards: nucleo_l476rg: enable MPU

d8111b3

Signed-off-by: Michel Jaouen <[email protected]>

boards: disco_l475_iot1: enable MPU

4316839

Signed-off-by: Michel Jaouen <[email protected]>

arm: soc: stm32: f3: add MPU capability

eae347e

Since not all socs from f3 series (i.e stm32f334x8 no MPU) have MPU capability, add capability only for MPU capable socs in Kconfig.soc JIRA: ZEP-2220 Signed-off-by: Michel Jaouen <[email protected]>

arm: stm32f1: Add support for STM32F103x8 SoC

e59a33d

Add configuration and memory definitions to support STM32F103x8 Medium-density performance line SoC with 64 KB Flash. Merge multiple files into single Kconfig.defconfig.stm32f103xx Signed-off-by: Siddharth Chandrasekaran <[email protected]>

mpu_stack_guard_test: update log in README.rst

1a620de

Following test changes, log info have changes and previous log info with stack guard present shows an mpu error not caused by a stack overflow caught by mpu stack guard feature. Signed-off-by: Michel Jaouen <[email protected]>

jamike force-pushed the fix_arm_mpu_guard_stack_alignement branch from 9cec2ca to 1a620de Compare June 27, 2017 09:59

agross-oss approved these changes Jun 28, 2017

View reviewed changes

MaureenHelm reviewed Jun 29, 2017

View reviewed changes

galak force-pushed the arm branch 4 times, most recently from f95c1d6 to 4c01e1a Compare June 30, 2017 20:53

agross-oss reviewed Jul 1, 2017

View reviewed changes

galak force-pushed the arm branch 3 times, most recently from 0f36cad to 1bc2fdc Compare July 7, 2017 15:32

andrewboie reviewed Jul 10, 2017

View reviewed changes

galak force-pushed the arm branch 4 times, most recently from 0723c92 to 38db4ed Compare July 19, 2017 16:37

MaureenHelm force-pushed the arm branch from 38db4ed to b4598c1 Compare July 28, 2017 19:55

agross-oss mentioned this pull request Aug 2, 2017

Fixup ARM MPU to use opaque types / Rebase of #500 #987

Merged

galak force-pushed the arm branch from fe0feea to 5398ef4 Compare August 8, 2017 16:24

galak closed this Aug 9, 2017

This was referenced Sep 18, 2017

stack declaration macros for ARM MPU zephyriot/zep-jira14#2100

Closed

stack declaration macros for ARM MPU zephyriot/zephyr-issues#2100

Closed

zephyrbot mentioned this pull request Sep 23, 2017

stack declaration macros for ARM MPU #3700

Closed

zephyrbot mentioned this pull request Sep 16, 2019

arc: hsdk: bug fixes and new feature supports #19173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mpu stack guard: fix stack alignement #500

mpu stack guard: fix stack alignement #500

jamike commented Jun 15, 2017

MaureenHelm Jun 15, 2017

galak Jun 16, 2017

MaureenHelm Jun 15, 2017

jamike Jun 16, 2017

MaureenHelm Jun 15, 2017

jamike Jun 16, 2017 •

edited

Loading

galak commented Jun 15, 2017

jamike commented Jun 16, 2017

dbkinder Jun 19, 2017

jamike Jun 27, 2017

dbkinder left a comment

andrewboie commented Jun 20, 2017

jamike commented Jun 28, 2017

galak commented Jun 28, 2017

MaureenHelm left a comment

MaureenHelm Jun 29, 2017

agross-oss Jul 1, 2017

MaureenHelm Jun 29, 2017

jamike Jun 30, 2017

agross-oss Jul 1, 2017

andrewboie Jul 10, 2017

andrewboie left a comment

agross-oss commented Aug 2, 2017

galak commented Aug 9, 2017

mpu stack guard: fix stack alignement #500

mpu stack guard: fix stack alignement #500

Conversation

jamike commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamike Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

galak commented Jun 15, 2017

jamike commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

andrewboie commented Jun 20, 2017

jamike commented Jun 28, 2017

galak commented Jun 28, 2017

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewboie left a comment

Choose a reason for hiding this comment

agross-oss commented Aug 2, 2017

galak commented Aug 9, 2017

jamike Jun 16, 2017 •

edited

Loading