tests: subsys: secure_storage: Erase flash before write once test #99701

petejohanson-adi · 2025-11-19T21:37:46Z

Where possible (i.e. not running in NS), erase the flash before testing the write once secure storage API.

This is based on feedback here: #97529 (review)

Note: This does not help with NS targets that can't directly access the flash and erase it. @tomi-font I'd really appreciate if you could offer some ideas for how to address that problem other than the one proposed in #97529

dsseng · 2025-11-19T21:45:16Z

Are NS targets even supposed to use secure_storage? I believe if you cannot access the flash from NS, then you would typically use PSA ITS/PS provided by the secure world, thus not needing Zephyr's implementation

petejohanson-adi · 2025-11-19T21:52:12Z

In our particular case, we have a possible convoluted path of secure_storage -> settings -> settings ITS backend.

dsseng · 2025-11-19T21:51:22Z

tests/subsys/secure_storage/psa/its/src/main.c

+#if defined(CONFIG_FLASH_HAS_NO_EXPLICIT_ERASE)
+	const struct flash_parameters *fparam = flash_get_parameters(fdev);
+
+	if (!(flash_params_get_erase_cap(fparam) & FLASH_ERASE_C_EXPLICIT)) {


Maybe in case we have no erase operation (e.g. nRF54 RRAM and MRAM) we should fully overwrite the region with zeroes or ones? (edit: or here means I did not verify which of this does Secure Storage consider to be empty, there's obviously a single variant it does) That would also erase the data to make sure we start from a known-empty state.

native_sim can also simulate such NVM model IIRC, so it should be testable even without such HW. Please take a look at native simulator flash Kconfig file for details, or if it's missing such a variant you might need to open an issue

Ok, so I've pushed an update, that among other things aims to get this to work for native_sim stock (which assumes explicit erases), and if you pass --extra-args CONFIG_FLASH_SIMULATOR_EXPLICIT_ERASE=n to twister.

For the no-explicit-erase scenario, I am using a flash_write to initialize all the pages to ones, which makes this pass if you re-run the twister tests with -n to preserve the directory which also has the side effect of preserving the binary file that's mmap'd in for the simulated support.

I'm not sure if this will also work on nRF54, I will dig up my XIAO nRF54L15 later today and see how that goes.

dsseng · 2025-11-19T21:51:43Z

tests/subsys/secure_storage/psa/its/src/main.c

+
+	for (int i = 0; i < MAX_NUM_PAGES && address < FLASH_ERASE_END_ADDR; i++) {
+		rc = flash_get_page_info_by_offs(fdev, address, &page_info);
+


non-blocking nit to unify the code style here

Suggested change

dsseng · 2025-11-19T21:54:24Z

In our particular case, we have a possible convoluted path of secure_storage -> settings -> settings ITS backend.

Yes, nothing prevents this from working, but perhaps only running this test without TF-M is sufficient for the coverage purposes

tomi-font

@dsseng is right in that NS targets are not meant to use Secure Storage:

zephyr/subsys/secure_storage/Kconfig

Lines 4 to 6 in f1c2ce4

    
           menuconfig SECURE_STORAGE 
        
           	bool "Secure storage subsystem" 
        
           	depends on !BUILD_WITH_TFM

However this test has a scenario for TF-M in order to test API compatibility between TF-M and Secure Storage (as all the test does is use the PSA ITS API):

zephyr/tests/subsys/secure_storage/psa/its/testcase.yaml

Lines 67 to 71 in f1c2ce4

    
           secure_storage.psa.its.tfm: 
        
             filter: CONFIG_BUILD_WITH_TFM 
        
             integration_platforms: 
        
               - nrf9151dk/nrf9151/ns 
        
             extra_args: EXTRA_CONF_FILE=overlay-tfm.conf

So it makes sense to also consider TF-M here.

In our particular case, we have a possible convoluted path of secure_storage -> settings -> settings ITS backend.

This is really weird... Can you tell me more about your use case? Why wouldn't you just directly be using the PSA ITS API exposed by TF-M, and how can you even manage to do this as there must be symbol clashes?

Other than that, thanks for the PR, I tested and it works on the nRF9151 and on native_sim with the Settings backend. Although not:

On the nRF54L15, probably because of https://github.com/zephyrproject-rtos/zephyr/pull/99701/files#r2543678217; could you also make it work there?

With the ZMS backend. In that case (on native_sim) the return value of psa_its_get_info() is not PSA_SUCCESS, which makes the test fail. This is maybe because the erase operation messes up ZMS or that the mounting needs to be done after erasing the flash:

zephyr/subsys/secure_storage/src/its/store/zms.c

Lines 22 to 34 in bd9dc90

    
           static int init_zms(void) 
        
           { 
        
           	int ret; 
        
           	s_zms.sector_count = FIXED_PARTITION_NODE_SIZE(PARTITION_DT_NODE) / s_zms.sector_size; 
        
           	ret = zms_mount(&s_zms); 
        
           	if (ret) { 
        
           		LOG_DBG("Failed. (%d)", ret); 
        
           	} 
        
           	return ret; 
        
           } 
        
           SYS_INIT(init_zms, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY);

Maybe having the flash erase as a SYS_INIT() hook that would run before that of ZMS would work.

Note: This does not help with NS targets that can't directly access the flash and erase it. @tomi-font I'd really appreciate if you could offer some ideas for how to address that problem other than the one proposed in #97529

I'm afraid there's no way around that, for NS targets you will just need to run twister/west with the erase option. It would be a pretty big security hole if the NS application could just erase parts of the secure flash. I mean I imagine it must be possible some way to achieve that but it must be all but trivial.

tests/subsys/secure_storage/psa/its/src/main.c

tomi-font · 2025-11-20T10:10:20Z

tests/subsys/secure_storage/psa/its/src/main.c

+	}
+}
+
+#endif


Suggested change

#endif

#endif /* !CONFIG_BUILD_WITH_TFM && CONFIG_FLASH_HAS_EXPLICIT_ERASE */

tests/subsys/secure_storage/psa/its/src/main.c

dsseng · 2025-11-20T11:25:57Z

I'm afraid there's no way around that, for NS targets you will just need to run twister/west with the erase option. It would be a pretty big security hole if the NS application could just erase parts of the secure flash. I mean I imagine it must be possible some way to achieve that but it must be all but trivial.

Can be potentially hacked around by making TF-M skip setting security attributes to the flash controller (needs a test-specific patch). It's still not really worth it for the test though it seems.

And on properly secured hardware that's the goal of TF-M to prevent NSPE from accessing Secure hardware, so there's no universal solution without hacking on TF-M.

@petejohanson-adi I still fail to grasp what do you test in such a case? If you use Secure Storage, then it writes to somewhere accessible from NSPE, right? Then that region should also be erasable/rewriteable by NSPE.

If you meant Secure Storage on top of ITS provided by TF-M (yes, I now realized it shouldn't even compile due to TF-M runtime library providing the same symbol names as secure_storage), please take a look here, maybe you should wipe ITS entries, and not the backing Flash: https://github.com/zephyrproject-rtos/zephyr/pull/94881/files#diff-93455f3e147570503f332b1446633676ffb36a58f958179f334273ca6b30c78e

petejohanson-adi · 2025-11-24T18:40:59Z

@dsseng is right in that NS targets are not meant to use Secure Storage:

zephyr/subsys/secure_storage/Kconfig

Lines 4 to 6 in f1c2ce4

menuconfig SECURE_STORAGE

bool "Secure storage subsystem"

depends on !BUILD_WITH_TFM

However this test has a scenario for TF-M in order to test API compatibility between TF-M and Secure Storage (as all the test does is use the PSA ITS API):

zephyr/tests/subsys/secure_storage/psa/its/testcase.yaml

Lines 67 to 71 in f1c2ce4

secure_storage.psa.its.tfm:

filter: CONFIG_BUILD_WITH_TFM

integration_platforms:

- nrf9151dk/nrf9151/ns

extra_args: EXTRA_CONF_FILE=overlay-tfm.conf

So it makes sense to also consider TF-M here.

In our particular case, we have a possible convoluted path of secure_storage -> settings -> settings ITS backend.

This is really weird... Can you tell me more about your use case? Why wouldn't you just directly be using the PSA ITS API exposed by TF-M, and how can you even manage to do this as there must be symbol clashes?

My apologies... That message was totally in error, from me incorrect quick assessment of the test case that was running, and getting mixed up with the failures we've been chasing on the non-NS targets. It is in fact the scenario you're highlighting, with the TFM test to verify compatibility.

Other than that, thanks for the PR, I tested and it works on the nRF9151 and on native_sim with the Settings backend. Although not:

On the nRF54L15, probably because of https://github.com/zephyrproject-rtos/zephyr/pull/99701/files#r2543678217; could you also make it work there?

I'll take a look, thanks.

With the ZMS backend. In that case (on native_sim) the return value of psa_its_get_info() is not PSA_SUCCESS, which makes the test fail. This is maybe because the erase operation messes up ZMS or that the mounting needs to be done after erasing the flash:

zephyr/subsys/secure_storage/src/its/store/zms.c

Lines 22 to 34 in bd9dc90

static int init_zms(void)

{

int ret;

s_zms.sector_count = FIXED_PARTITION_NODE_SIZE(PARTITION_DT_NODE) / s_zms.sector_size;

ret = zms_mount(&s_zms);

if (ret) {

LOG_DBG("Failed. (%d)", ret);

}

return ret;

}

SYS_INIT(init_zms, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY);

Maybe having the flash erase as a SYS_INIT() hook that would run before that of ZMS would work.

Yeah, I was a bit worried about that possible case. I'm fine moving it to an earlier SYS_INIT hook, that's probably more reliable across the board.

Note: This does not help with NS targets that can't directly access the flash and erase it. @tomi-font I'd really appreciate if you could offer some ideas for how to address that problem other than the one proposed in #97529

I'm afraid there's no way around that, for NS targets you will just need to run twister/west with the erase option. It would be a pretty big security hole if the NS application could just erase parts of the secure flash. I mean I imagine it must be possible some way to achieve that but it must be all but trivial.

Thanks, that was exactly my take as well, but unfortunately, running a whole twister suite with the --erase option for every test has a non-trivial impact to the total test time (the bulk erase time adds up), which is why in #97529 we had tried to suggest a full erase just for this test. I understand why many that may not be desirable for all targets... Thoughts adding that erase to the runners for this test, but conditional on a CMake define that folks can use to opt into that behavior? @tomi-font ?

Where possible (i.e. not running in NS), erase the flash before testing the write once secure storage API. Signed-off-by: Pete Johanson <[email protected]>

sonarqubecloud · 2025-11-25T01:46:12Z

Quality Gate passed

Issues
0 New issues
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

tomi-font · 2025-11-25T09:01:33Z

Thanks, that was exactly my take as well, but unfortunately, running a whole twister suite with the --erase option for every test has a non-trivial impact to the total test time (the bulk erase time adds up), which is why in #97529 we had tried to suggest a full erase just for this test. I understand why many that may not be desirable for all targets... Thoughts adding that erase to the runners for this test, but conditional on a CMake define that folks can use to opt into that behavior? @tomi-font ?

I see, I wouldn't be against for example adding a Kconfig option to that test to allow opting in if the approach proposed in #97529 would be acceptable in that case. @nordicjm @de-nordic thoughts?

tomi-font · 2025-11-25T09:15:29Z

tests/subsys/secure_storage/psa/its/src/main.c

+
+		memset(empty, 0xff, page_info.size);
+
+		TC_PRINT("Setting %d at %ld to 0xff\n", page_info.size, page_info.start_offset);


Suggested change

TC_PRINT("Setting %d at %ld to 0xff\n", page_info.size, page_info.start_offset);

TC_PRINT("Setting %zu at %ld to 0xff\n", page_info.size, page_info.start_offset);

tomi-font · 2025-11-25T09:20:58Z

tests/subsys/secure_storage/psa/its/src/main.c

+
+#if !defined(CONFIG_BUILD_WITH_TFM) && defined(CONFIG_FLASH_PAGE_LAYOUT)
+
+#define MAX_NUM_PAGES 2


Suggested change

#define MAX_NUM_PAGES 2

tomi-font · 2025-11-25T09:21:49Z

tests/subsys/secure_storage/psa/its/src/main.c

+
+#if defined(CONFIG_FLASH_HAS_NO_EXPLICIT_ERASE)
+		/* Lacking an explicit erase, just write all ones to the pages */
+		uint8_t empty[page_info.size];


VLAs are not allowed

Ok, I'll adjust and use a fixed size array and iterate.

tomi-font · 2025-11-25T09:24:17Z

tests/subsys/secure_storage/psa/its/src/main.c

+			return rc;
+		}
+#else
+		TC_PRINT("Erasing %d at %ld\n", page_info.size, page_info.start_offset);


Suggested change

TC_PRINT("Erasing %d at %ld\n", page_info.size, page_info.start_offset);

TC_PRINT("Erasing %zu at %ld\n", page_info.size, page_info.start_offset);

tomi-font · 2025-11-25T09:26:15Z

tests/subsys/secure_storage/psa/its/src/main.c

+		if (rc < 0) {
+			return rc;
+		}
+#endif


Suggested change

#endif

#endif /* CONFIG_FLASH_HAS_NO_EXPLICIT_ERASE */

de-nordic · 2025-11-25T09:29:20Z

tests/subsys/secure_storage/psa/its/src/main.c

+			return rc;
+		}
+
+#if defined(CONFIG_FLASH_HAS_NO_EXPLICIT_ERASE)


Avoid using Flash API Kconfig in code that is not Flash API. Make test/subsystem specific Kconfig dependent on the Flash API kconfig.

de-nordic · 2025-11-25T09:30:25Z

I see, I wouldn't be against for example adding a Kconfig option to that test to allow opting in if the approach proposed in #97529 would be acceptable in that case. @nordicjm @de-nordic thoughts?

I am ok with adding Kconfig to control this, I am not ok with directly using Flash API kconfig in code, it can be used in test Kconfig.

tomi-font · 2025-11-25T09:35:17Z

I am ok with adding Kconfig to control this, I am not ok with directly using Flash API kconfig in code, it can be used in test Kconfig.

Do you mean to do

config MY_TEST_KCONFIG
        def_bool FLASH_HAS_NO_EXPLICIT_ERASE

? If so, why?

de-nordic · 2025-11-25T10:03:25Z

I am ok with adding Kconfig to control this, I am not ok with directly using Flash API kconfig in code, it can be used in test Kconfig.

Do you mean to do
config MY_TEST_KCONFIG
        def_bool FLASH_HAS_NO_EXPLICIT_ERASE
? If so, why?

Because the Flash API Kconfig belongs to Flash API. If I, or anyone else, needs to change that Kconfig for some reason I would have to go through a lot of subsystems code to fix it being used there to. If the Kconfig is only driving other Kconfigs then I only need to go through Kconfigs.
Additionally, this Kconfig is just informing that there is certain type of a device in system; subsystem may or may not support it, or have specific option to select support for such a device, and that options should be dependent on the Flash API Kconfig; because that option actually controls behavior and paths within subsystem, no Flash API.

tomi-font · 2025-11-25T10:38:01Z

Because the Flash API Kconfig belongs to Flash API. If I, or anyone else, needs to change that Kconfig for some reason I would have to go through a lot of subsystems code to fix it being used there to. If the Kconfig is only driving other Kconfigs then I only need to go through Kconfigs.

I don't know if I'm missing something but if a Kconfig option is renamed then it's the same thing to rename it in Kconfig or source files.

Additionally, this Kconfig is just informing that there is certain type of a device in system; subsystem may or may not support it, or have specific option to select support for such a device, and that options should be dependent on the Flash API Kconfig; because that option actually controls behavior and paths within subsystem, no Flash API.

Do you mean the flash subsystem may not support it or some other subsystem that makes use of the flash API? In the case of this PR it's purely about a test that is directly using the flash API to manipulate the NVM.

de-nordic · 2025-11-25T18:29:48Z

Do you mean the flash subsystem may not support it or some other subsystem that makes use of the flash API? In the case of this PR it's purely about a test that is directly using the flash API to manipulate the NVM.

Example from Stream Flash

zephyr/subsys/storage/stream/Kconfig

Lines 32 to 37 in c1d16c0

    
           config STREAM_FLASH_ERASE 
        
           	bool "Perform erase operations" 
        
           	depends on FLASH_HAS_EXPLICIT_ERASE 
        
           	help 
        
           	  If disabled an external actor must erase the flash area being written 
        
           	  to.

the above option only appears if there is at least one device with explicit erase. User may select it or not, depending whether user intends to use the subsystem on device with this characteristic.

If you assume that Kconfig is itself a separate configuration module, consisting of sub-modules steering subsystems that will be compiled in, then these sub-modules should only control sub-modules they have been developed for. So even if flash Kconfigs may have direct impact on other subsystem configuration, for example Stream Flash, the should not have direct usage in subsystems they have not been designed to control.

This is my opinion and I may be wrong here, but imho it looks better in code if it is controlled, even in compilation, by identifiers directly intended for that subsystem/module.

petejohanson-adi · 2025-11-25T19:46:44Z

Do you mean the flash subsystem may not support it or some other subsystem that makes use of the flash API? In the case of this PR it's purely about a test that is directly using the flash API to manipulate the NVM.

Example from Stream Flash

zephyr/subsys/storage/stream/Kconfig

Lines 32 to 37 in c1d16c0

config STREAM_FLASH_ERASE

bool "Perform erase operations"

depends on FLASH_HAS_EXPLICIT_ERASE

help

If disabled an external actor must erase the flash area being written

to.

the above option only appears if there is at least one device with explicit erase. User may select it or not, depending whether user intends to use the subsystem on device with this characteristic.

If you assume that Kconfig is itself a separate configuration module, consisting of sub-modules steering subsystems that will be compiled in, then these sub-modules should only control sub-modules they have been developed for. So even if flash Kconfigs may have direct impact on other subsystem configuration, for example Stream Flash, the should not have direct usage in subsystems they have not been designed to control.

This is my opinion and I may be wrong here, but imho it looks better in code if it is controlled, even in compilation, by identifiers directly intended for that subsystem/module.

I've locally adjusted the code to just check flash_params_get_erase_cap(fparam) for the FLASH_ERASE_C_EXPLICIT flag, and then deciding based on that how in the test to try to clear things out. Is this more aligned with your expectations? It's no longer using those "internal" Kconfig symbols to determine how to proceed.

zephyrbot added area: Secure Storage Secure Storage area: Tests Issues related to a particular existing or missing test labels Nov 19, 2025

zephyrbot requested review from nashif and tomi-font November 19, 2025 21:39

zephyrbot assigned tomi-font Nov 19, 2025

dsseng reviewed Nov 19, 2025

View reviewed changes

tomi-font reviewed Nov 20, 2025

View reviewed changes

tests: subsys: secure_storage: Erase flash before write once test

6f86b85

Where possible (i.e. not running in NS), erase the flash before testing the write once secure storage API. Signed-off-by: Pete Johanson <[email protected]>

petejohanson-adi force-pushed the testing/secure-storage-flash-erase-tweaks branch from ec97597 to 6f86b85 Compare November 25, 2025 01:20

tomi-font requested changes Nov 25, 2025

View reviewed changes

de-nordic reviewed Nov 25, 2025

View reviewed changes


		for (int i = 0; i < MAX_NUM_PAGES && address < FLASH_ERASE_END_ADDR; i++) {
		rc = flash_get_page_info_by_offs(fdev, address, &page_info);

	menuconfig SECURE_STORAGE
	bool "Secure storage subsystem"
	depends on !BUILD_WITH_TFM

	secure_storage.psa.its.tfm:
	filter: CONFIG_BUILD_WITH_TFM
	integration_platforms:
	- nrf9151dk/nrf9151/ns
	extra_args: EXTRA_CONF_FILE=overlay-tfm.conf

	static int init_zms(void)
	{
	int ret;

	s_zms.sector_count = FIXED_PARTITION_NODE_SIZE(PARTITION_DT_NODE) / s_zms.sector_size;

	ret = zms_mount(&s_zms);
	if (ret) {
	LOG_DBG("Failed. (%d)", ret);
	}
	return ret;
	}
	SYS_INIT(init_zms, APPLICATION, CONFIG_APPLICATION_INIT_PRIORITY);

+              	}
+              }
+              #endif

	#endif
	#endif /* !CONFIG_BUILD_WITH_TFM && CONFIG_FLASH_HAS_EXPLICIT_ERASE */


		memset(empty, 0xff, page_info.size);

		TC_PRINT("Setting %d at %ld to 0xff\n", page_info.size, page_info.start_offset);

	TC_PRINT("Setting %d at %ld to 0xff\n", page_info.size, page_info.start_offset);
	TC_PRINT("Setting %zu at %ld to 0xff\n", page_info.size, page_info.start_offset);


		#if !defined(CONFIG_BUILD_WITH_TFM) && defined(CONFIG_FLASH_PAGE_LAYOUT)

		#define MAX_NUM_PAGES 2

	TC_PRINT("Erasing %d at %ld\n", page_info.size, page_info.start_offset);
	TC_PRINT("Erasing %zu at %ld\n", page_info.size, page_info.start_offset);

tests: subsys: secure_storage: Erase flash before write once test #99701

Are you sure you want to change the base?

tests: subsys: secure_storage: Erase flash before write once test #99701

Conversation

petejohanson-adi commented Nov 19, 2025

Uh oh!

dsseng commented Nov 19, 2025

Uh oh!

petejohanson-adi commented Nov 19, 2025

Uh oh!

dsseng Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsseng commented Nov 19, 2025

Uh oh!

tomi-font left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsseng commented Nov 20, 2025

Uh oh!

petejohanson-adi commented Nov 24, 2025

Uh oh!

sonarqubecloud bot commented Nov 25, 2025

Quality Gate passed

Uh oh!

tomi-font commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-nordic commented Nov 25, 2025

Uh oh!

tomi-font commented Nov 25, 2025

Uh oh!

de-nordic commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomi-font commented Nov 25, 2025

Uh oh!

de-nordic commented Nov 25, 2025

Uh oh!

petejohanson-adi commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dsseng Nov 19, 2025 •

edited

Loading

de-nordic commented Nov 25, 2025 •

edited

Loading