secure_services: Lock scheduler before calling secure services. #2519

oyvindronningstad · 2020-06-23T11:27:48Z

call k_sched_lock before and k_sched_unlock after.

Rename the services from spm_foo() to spm_foo_nsc() and call
spm_foo_nsc() from spm_foo() along with k_sched_*()

Ref: NCSIDB-108

Signed-off-by: Øyvind Rønningstad oyvind.ronningstad@nordicsemi.no

oyvindronningstad · 2020-06-24T14:04:13Z

If zephyrproject-rtos/zephyr#26414 goes through, this PR should be changed to accommodate

NordicBuilder · 2020-06-24T14:17:59Z

Some checks failed. Please fix and resubmit.

checkpatch issues

No additional types will be considered - file 'scripts/checkpatch/typedefsfile': No such file or directory
-:256: ERROR:INITIALISED_STATIC: do not initialise statics to false
#256: FILE: tests/subsys/spm/thread_swap/src/main.c:12:
+static volatile bool work_done = false;

- total: 1 errors, 0 warnings, 245 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

ioannisg

I would believe that the scheduler lock needs to happen before the secure call (at least this is how I described the proposed solution in the respective ticket). Otherwise we do not completely get rid of race conditions. [Might be missing something so let's discuss.]

oyvindronningstad · 2020-06-24T16:50:40Z

I would believe that the scheduler lock needs to happen before the secure call (at least this is how I described the proposed solution in the respective ticket). Otherwise we do not completely get rid of race conditions. [Might be missing something so let's discuss.]

It does happen before the secure call. The _nsc functions are the entry functions.

ioannisg · 2020-06-24T17:20:16Z

Ok, thanks, I saw it now. So the patch should be functionally fine, however I think it would be better to embed the cooperative scheduling enforcement in the way the Secure calls are defined. So you would not need to duplicate the kernel locking in every secure service call. And that patch could actually be sent upstream directly!

ioannisg · 2020-10-12T10:03:25Z

Ok, thanks, I saw it now. So the patch should be functionally fine, however I think it would be better to embed the cooperative scheduling enforcement in the way the Secure calls are defined. So you would not need to duplicate the kernel locking in every secure service call. And that patch could actually be sent upstream directly!

This work was merged upstream in zephyrproject-rtos/zephyr#27831 and should be part of v2.4.0. I believe @oyvindronningstad has updated the current PR to take the upstream changes into account.

lemrey

Thanks, this fixed the issue I was seeing when calling sys_rand32_get.
I wonder if it would be better to infix _ncs_ instead e.g. spm_ncs_foo, no strong opinion though.

nit: can you remove the punctuation from the commit title

subsys/spm/secure_services_ns.c

subsys/CMakeLists.txt

subsys/spm/CMakeLists.txt

subsys/spm/Kconfig

subsys/spm/secure_services_ns.c

hakonfam · 2020-10-12T11:05:01Z

subsys/spm/secure_services_ns.c

+int spm_prevalidate_b1_upgrade_nsc(uint32_t dst_addr, uint32_t src_addr);
+void spm_busy_wait_nsc(uint32_t busy_wait_us);
+
+TZ_THREAD_SAFE_NONSECURE_ENTRY_FUNC(spm_request_system_reboot,


I guess that the commit message was composed before this functionality was available in zephyr, so you should check that it is up to date.

call k_sched_lock before and k_sched_unlock after. Rename the services from spm_foo() to spm_foo_nsc() and call spm_foo_nsc() from spm_foo() along with k_sched_*()

There is no mention of k_sched in this PR since it is all handled in the TZ macros.

and by the way, you should elaborate here why THREAD_SAFE is needed (maybe some simple inline comments above the definitions)

Reworded the commit message. I didn't add anything about thread safety here, but I added docs elsewhere.

include/secure_services.h

hakonfam · 2020-10-12T11:06:17Z

subsys/spm/Kconfig

+config SPM_SERVICE_BUSY_WAIT
+	bool "Busy wait in secure mode (debug function)"
+	default y
+	help


Could it depend on ZTEST so that its not visible for customers when running menuconfig on a normal project?

Yes this is an option. (or TEST)
How about samples, where we would like to have this set by default as well?

Sadly, this config (in SPM) cannot depend on a config in the app.

But what if we modify the build system to pass "CONFIG_ZTEST" to any child image? In my opinion that is a reasonable feature. If you agree we can create a task and add it to the backlog.

I spent a bit of time thinking about this yesterday. I had a similar thought. We have the option of passing -Dspm_CONFIG_TEST=y-style Kconfig overrides from app to spm, but it's quite blunt, and means that those configs will be ignored if set with e.g. ninja spm_menuconfig.

I also considered just doing it with the SPM_SERVICE configs since they anyway have particular requirements about being in sync across app and spm. Doing this would solve another problem as well, which I fix in #2710, but I like this solution better.

The SPM_SERVICE_* configs are now passed from app to spm

hakonfam · 2020-10-12T11:21:52Z

tests/subsys/spm/thread_swap/testcase.yaml

@@ -0,0 +1,4 @@
+tests:
+  spm.secure_services.thread_swap:
+    platform_allow: nrf9160dk_nrf9160ns nrf5340pdk_nrf5340_cpuappns


Suggested change

platform_allow: nrf9160dk_nrf9160ns nrf5340pdk_nrf5340_cpuappns

platform_allow: nrf9160dk_nrf9160ns nrf5340pdk_nrf5340_cpuappns nrf5340dk_nrf5340_cpuappns

Not really sure if this is needed, just check what is in the other tests.

Looks like nrf5340dk_nrf5340_cpuappns is not used in any testcase.yaml or sample.yaml files yet.

tests/subsys/spm/thread_swap/src/main.c

tejlmand · 2020-10-12T18:13:54Z

@oyvindronningstad retriggered CI by forced push: 22d3fb81a24121f42793dba6fa84cf54fe3688de --> 22d3fb81a24121f42793dba6fa84cf54fe3688de

hakonfam · 2020-10-12T19:09:29Z

Rebased on latest master and force pushed from 22d3fb8 to 10e6282 to try and fix CI.

hakonfam · 2020-10-12T19:31:28Z

Hmm, look like there's legit failures in CI

/opt/gnuarmemb/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: zephyr/libzephyr.a(secure_services_ns.c.obj): in function `spm_request_system_reboot':
/jenkins_workspace/workspace/nrfconnect-boot-fw-update_master/test_objects/nrf/subsys/spm/secure_services_ns.c:18: undefined reference to `spm_request_system_reboot_nsc'
/opt/gnuarmemb/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: zephyr/libzephyr.a(secure_services_ns.c.obj): in function `spm_prevalidate_b1_upgrade':
/jenkins_workspace/workspace/nrfconnect-boot-fw-update_master/test_objects/nrf/subsys/spm/secure_services_ns.c:33: undefined reference to `spm_prevalidate_b1_upgrade_nsc'
collect2: error: ld returned 1 exit status

oyvindronningstad · 2020-10-13T13:48:14Z

Hmm, look like there's legit failures in CI

/opt/gnuarmemb/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: zephyr/libzephyr.a(secure_services_ns.c.obj): in function `spm_request_system_reboot':
/jenkins_workspace/workspace/nrfconnect-boot-fw-update_master/test_objects/nrf/subsys/spm/secure_services_ns.c:18: undefined reference to `spm_request_system_reboot_nsc'
/opt/gnuarmemb/gcc-arm-none-eabi-9-2019-q4-major/bin/../lib/gcc/arm-none-eabi/9.2.1/../../../../arm-none-eabi/bin/ld: zephyr/libzephyr.a(secure_services_ns.c.obj): in function `spm_prevalidate_b1_upgrade':
/jenkins_workspace/workspace/nrfconnect-boot-fw-update_master/test_objects/nrf/subsys/spm/secure_services_ns.c:33: undefined reference to `spm_prevalidate_b1_upgrade_nsc'
collect2: error: ld returned 1 exit status

I couldn't reproduce this locally with samples, so the test might be misconfigured. Will look more into later.

samples/CMakeLists.txt

tejlmand

Some small nits, and one possible flaw but not necessarily blocking for merging.

include/secure_services.h

samples/CMakeLists.txt

tejlmand · 2020-10-14T10:54:21Z

samples/CMakeLists.txt

+  list(FILTER VARIABLES_CACHED INCLUDE REGEX ${regex})
+
+  foreach(var_name ${VARIABLES} ${VARIABLES_CACHED})
+    set(spm_${var_name} ${${var_name}})


So, if user did: -Dspm_CONFIG_SPM_SERVICE<something>=<value> that will be lost and overwritten with the value of CONFIG_SPM_SERVICE<something>.

Sounds like a flaw.

In the case of these configs, any mismatch in configuration between SPM and app is an error, so in this case, it's "desired". Ideally, there should be a way to prevent setting the config in SPM, but I opted to just mention it in the help text.

accepted.
Probably we should warn the user then, in case he has already defined spm_CONFIG_SPM_SERVICE<something> before overwriting the value.

But that can be done as enhancement.

tests/subsys/spm/thread_swap/src/main.c

oyvindronningstad · 2020-10-14T11:29:59Z

Thanks, this fixed the issue I was seeing when calling sys_rand32_get.
I wonder if it would be better to infix _ncs_ instead e.g. spm_ncs_foo, no strong opinion though.

Thanks. This is now used via concatenation though, so infix is not really a convenient alternative.

nit: can you remove the punctuation from the commit title

Thanks, done

Rename the services from spm_foo() to spm_foo_nsc() and create new wrapper functions spm_foo() k_sched_*) using TZ_THREAD_SAFE_NONSECURE_ENTRY_FUNC(). Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

The service busy waits in secure mode, so we can test e.g. thread swapping between secure and nonsecure. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

This test verifies that secure services can be safely interrupted by a thread swap that also calls a secure service. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

This is now the preferred way, to keep the configs in sync. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

subsys/spm/secure_services.c

frkv

LGTM

Stale, comments himself afterwards that the reason he NACKed was a misunderstanding.

oyvindronningstad requested review from hakonfam, ioannisg, AryanNordic and frkv June 23, 2020 11:27

oyvindronningstad requested review from anangl and rlubos as code owners June 23, 2020 11:27

oyvindronningstad added the DNM label Jun 24, 2020

oyvindronningstad requested a review from rakons as a code owner June 24, 2020 14:14

oyvindronningstad force-pushed the spm-sched-lock branch 2 times, most recently from b6121ba to 1b0f329 Compare June 24, 2020 14:17

oyvindronningstad force-pushed the spm-sched-lock branch 2 times, most recently from 9578a3a to 96b306a Compare June 24, 2020 14:20

ioannisg previously requested changes Jun 24, 2020

View reviewed changes

oyvindronningstad force-pushed the spm-sched-lock branch from 96b306a to 3099808 Compare September 7, 2020 08:55

oyvindronningstad force-pushed the spm-sched-lock branch from 3099808 to 1ceeedb Compare October 7, 2020 17:11

oyvindronningstad requested a review from ioannisg October 7, 2020 17:11

oyvindronningstad force-pushed the spm-sched-lock branch from 1ceeedb to 72c3fb5 Compare October 7, 2020 17:36

oyvindronningstad removed the DNM label Oct 7, 2020

oyvindronningstad force-pushed the spm-sched-lock branch from 72c3fb5 to 22d3fb8 Compare October 8, 2020 17:52

shantha-14 added this to the 1.4.0 milestone Oct 12, 2020

lemrey approved these changes Oct 12, 2020

View reviewed changes

hakonfam reviewed Oct 12, 2020

View reviewed changes

tejlmand requested a review from nordic-hani October 12, 2020 13:21

nordic-hani approved these changes Oct 12, 2020

View reviewed changes

tejlmand force-pushed the spm-sched-lock branch from 22d3fb8 to 8ed74a7 Compare October 12, 2020 18:10

tejlmand force-pushed the spm-sched-lock branch from 8ed74a7 to 22d3fb8 Compare October 12, 2020 18:11

hakonfam force-pushed the spm-sched-lock branch from 22d3fb8 to 10e6282 Compare October 12, 2020 19:08

shantha-14 self-assigned this Oct 13, 2020

oyvindronningstad force-pushed the spm-sched-lock branch from 10e6282 to 4527812 Compare October 13, 2020 13:39

oyvindronningstad requested a review from tejlmand as a code owner October 13, 2020 13:39

hakonfam reviewed Oct 13, 2020

View reviewed changes

samples/CMakeLists.txt Outdated Show resolved Hide resolved

oyvindronningstad force-pushed the spm-sched-lock branch 2 times, most recently from dc9d60c to 15af30d Compare October 14, 2020 08:22

tejlmand reviewed Oct 14, 2020

View reviewed changes

oyvindronningstad force-pushed the spm-sched-lock branch from 15af30d to 23833ba Compare October 14, 2020 11:27

oyvindronningstad added 4 commits October 14, 2020 13:55

secure_services: Add busy_wait service to be used for testing

e7e43e5

The service busy waits in secure mode, so we can test e.g. thread swapping between secure and nonsecure. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

tests: Add a thread swapping test for secure secure services

feb3d99

This test verifies that secure services can be safely interrupted by a thread swap that also calls a secure service. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

secure_services: Allow secure services to be enabled from the app

9e39d93

This is now the preferred way, to keep the configs in sync. Ref: NCSIDB-108 Signed-off-by: Øyvind Rønningstad <oyvind.ronningstad@nordicsemi.no>

frkv reviewed Oct 14, 2020

View reviewed changes

subsys/spm/secure_services.c Outdated Show resolved Hide resolved

oyvindronningstad force-pushed the spm-sched-lock branch from 23833ba to 9e39d93 Compare October 14, 2020 12:05

frkv self-requested a review October 14, 2020 12:24

frkv approved these changes Oct 14, 2020

View reviewed changes

tejlmand approved these changes Oct 14, 2020

View reviewed changes

hakonfam approved these changes Oct 14, 2020

View reviewed changes

tejlmand requested a review from shantha-14 October 15, 2020 14:01

shantha-14 approved these changes Oct 15, 2020

View reviewed changes

tejlmand merged commit 5744564 into nrfconnect:master Oct 15, 2020

jtguggedal mentioned this pull request Oct 19, 2020

lib: bin: lwm2m_carrier: update to version 0.10.0 #3051

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

secure_services: Lock scheduler before calling secure services. #2519

secure_services: Lock scheduler before calling secure services. #2519

oyvindronningstad commented Jun 23, 2020

oyvindronningstad commented Jun 24, 2020

NordicBuilder commented Jun 24, 2020 •

edited

ioannisg left a comment

oyvindronningstad commented Jun 24, 2020

ioannisg commented Jun 24, 2020

ioannisg commented Oct 12, 2020

lemrey left a comment

hakonfam Oct 12, 2020

ioannisg Oct 12, 2020

oyvindronningstad Oct 13, 2020

hakonfam Oct 12, 2020

ioannisg Oct 12, 2020

oyvindronningstad Oct 12, 2020

hakonfam Oct 12, 2020

oyvindronningstad Oct 13, 2020

oyvindronningstad Oct 13, 2020

hakonfam Oct 12, 2020

hakonfam Oct 12, 2020

oyvindronningstad Oct 14, 2020

tejlmand commented Oct 12, 2020

hakonfam commented Oct 12, 2020

hakonfam commented Oct 12, 2020

oyvindronningstad commented Oct 13, 2020

tejlmand left a comment

tejlmand Oct 14, 2020

oyvindronningstad Oct 14, 2020

tejlmand Oct 14, 2020

oyvindronningstad commented Oct 14, 2020

frkv left a comment

	platform_allow: nrf9160dk_nrf9160ns nrf5340pdk_nrf5340_cpuappns
	platform_allow: nrf9160dk_nrf9160ns nrf5340pdk_nrf5340_cpuappns nrf5340dk_nrf5340_cpuappns

secure_services: Lock scheduler before calling secure services. #2519

secure_services: Lock scheduler before calling secure services. #2519

Conversation

oyvindronningstad commented Jun 23, 2020

oyvindronningstad commented Jun 24, 2020

NordicBuilder commented Jun 24, 2020 • edited

checkpatch issues

ioannisg left a comment

Choose a reason for hiding this comment

oyvindronningstad commented Jun 24, 2020

ioannisg commented Jun 24, 2020

ioannisg commented Oct 12, 2020

lemrey 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

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

tejlmand commented Oct 12, 2020

hakonfam commented Oct 12, 2020

hakonfam commented Oct 12, 2020

oyvindronningstad commented Oct 13, 2020

tejlmand 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

oyvindronningstad commented Oct 14, 2020

frkv left a comment

Choose a reason for hiding this comment

NordicBuilder commented Jun 24, 2020 •

edited