sys/fido2: improve & simplify flash handling #21110

Ollrogge · 2024-12-25T16:01:12Z

Contribution description

This PR tries to improve the flash handling done by the FIDO2 implementation. Specifically this entails:

Usage of mtd_aux device to reserve flash memory for the FIDO2 data
No more usage of functionality that requires flash to be memory-mapped (e.g. _flash_is_erased function)
Coherent usage of the mtd_flashpage api

Since the AUX slot approach is integrated into the mtd API, which is the recommended method for working with flash, I wonder if the periph_flashpage_in_address_space feature I added to reserve flash memory for FIDO2 can now be deprecated ?

@benpicco, with the mtd_aux approach, how can two different applications reserve flash memory without risking corruption of each other's data? Is it possible to divide the AUX slot into multiple smaller slots, that can be allocated for use by different applications?

Testing procedure

/tests/sys/fido2_ctap with both native and nrf52840dk as target
/tests/sys/fido2_ctap_hid

riot-ci · 2025-01-28T17:55:33Z

Murdock results

✔️ PASSED

fe73a92 fixup! sys/fido2: improve & simplify flash handling

Success	Failures	Total	Runtime
10271	0	10271	09m:41s

Artifacts

Documentation preview

mguetschow

Thanks, some comments below. I'm not very familiar with the mtd API, maybe @benpicco could have a look as well?

mguetschow · 2025-01-28T17:40:15Z

sys/fido2/ctap/ctap.c

@@ -15,6 +15,7 @@
 * @}
 */

+#include "fido2/ctap/ctap.h"


Why is there fido2/ctap/ctap.h and fido2/ctap.h below?

The file fido2/ctap/ctap.h contains definitions that are internal to the CTAP module, while fido2/ctap.h provides definitions intended for external modules that interact with the FIDO2 API.

mguetschow · 2025-01-28T17:41:00Z

sys/fido2/ctap/ctap.c

+#define ENABLE_DEBUG (0)
 #include "debug.h"


Suggested change

#define ENABLE_DEBUG (0)

#include "debug.h"

#define ENABLE_DEBUG 0

#include "debug.h"

mguetschow · 2025-01-28T17:43:07Z

sys/fido2/ctap/ctap.c

@@ -424,10 +425,12 @@ static uint32_t get_id(void)

 static int _reset(void)
 {
-    int ret = fido2_ctap_mem_erase_flash();
+    if (_state.initialized_marker == CTAP_INITIALIZED_MARKER) {


Suggested change

if (_state.initialized_marker == CTAP_INITIALIZED_MARKER) {

if (_state.initialized_marker != CTAP_INITIALIZED_MARKER) {

or could you explain the semantics of CTAP_INITIALIZED_MARKER, maybe with a comment?

mguetschow · 2025-01-28T17:45:24Z

sys/fido2/ctap/ctap_mem.c

 #include "mtd_flashpage.h"

 #include "fido2/ctap/ctap_mem.h"
 #include "fido2/ctap/ctap_utils.h"

-#define ENABLE_DEBUG    (0)
+#define ENABLE_DEBUG (0)


Suggested change

#define ENABLE_DEBUG (0)

#define ENABLE_DEBUG 0

(as suggested by the static tests)

mguetschow · 2025-01-28T17:46:59Z

sys/fido2/ctap/ctap_mem.c


 ctap_status_code_t fido2_ctap_mem_init(void)
 {
 #ifdef BOARD_NATIVE
    _mtd_dev = mtd_default_get_dev(0);
+#else
+    _mtd_dev = mtd_aux;


Where does mtd_aux come from?

This device comes from the periph_flashpage_aux feature which allows to reserve flash memory by using the auxiliary flash slot

mguetschow · 2025-01-28T17:50:43Z

sys/fido2/ctap/ctap_mem.c


-    int ret = mtd_erase(_mtd_dev, addr, sector_size * CONFIG_FIDO2_CTAP_NUM_FLASHPAGES);
+    int ret = mtd_erase_sector(_mtd_dev, 0, sector_cnt);


are we sure this would not clash with other usages of the same mtd device?

Yes I think this would happen. Hence also the question to @benpicco if there is a way to divide the aux slot somehow such that two applications can use it without risking to corrupt each others data

We don't have such mechanism for MTD, I'd say this is out of scope for this PR.

I think there really can't be an automatic solution for this - a concrete application would depend on the MTD device (and size) used anyway.

One could use mtd_mapper to share a MTD device, but again this is out of scope - using the MTD abstraction already helps a lot with flexibility.

mguetschow · 2025-01-28T17:53:22Z

sys/fido2/ctap/ctap_mem.c

-                                                     uint32_t *addr)
+ctap_status_code_t fido2_ctap_mem_read_rk_from_flash(ctap_resident_key_t *key,
+                                                     const uint8_t *rp_id_hash,
+                                                     uint32_t *off)


Is there a reason you don't pass the off by value?

This is done to enable going through all the stored fido2 credentials one by one as done here.

I could also change the function to use callback but this would increase complexity I think.

mguetschow · 2025-01-28T17:53:50Z

sys/fido2/ctap/ctap_mem.c

        end = amt_stored;
-        *addr = _flash_start_addr() + FLASHPAGE_SIZE;
+        /* skip ctap_state_t struct */


I am skipping the ctap_state_t struct since i am reading resident keys from flash in this function and it doesn't make sense to read the state struct for this

mguetschow · 2025-01-28T17:56:15Z

sys/include/fido2/ctap/ctap_mem.h

 *
 * @param[in]  key       pointer to authenticator state
 * @param[in]   rp_id_hash pointer to hash of rp domain string
- * @param[in] addr pointer to address where to read from
+ * @param[in] absolute_offset pointer to a variable holding the total offset from the


the parameter is called off in the implementation. Would be nice to align both.

From an API perspective, I would be nicer to have some iterator struct, that hides the offset information from the user.

Sounds good, do you have an example for an iterator struct somewhere in RIOT ?

benpicco · 2025-01-30T18:00:23Z

sys/fido2/Makefile.dep

@@ -5,14 +5,11 @@ ifneq (,$(filter fido2_ctap_%,$(USEMODULE)))
      USEMODULE += ztimer64_msec
      USEMODULE += usbus_hid
      DISABLE_MODULE += auto_init_usbus
+      FEATURES_REQUIRED += periph_flashpage_aux


I'd put that into the application and make the MTD device configurable for the module.

benpicco · 2025-01-30T18:03:04Z

sys/fido2/ctap/ctap_mem.c

 #include "debug.h"

+static mtd_dev_t *_mtd_dev;


Suggested change

static mtd_dev_t *_mtd_dev;

static mtd_dev_t *_mtd_dev = CONFIG_FIDO2_MTD_DEV;

We should make the application select this

benpicco · 2025-01-30T18:05:22Z

sys/fido2/ctap/ctap_mem.c

+    uint32_t used_pages = (CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ +
+                           _mtd_dev->page_size - 1) / _mtd_dev->page_size;


Suggested change

uint32_t used_pages = (CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ +

_mtd_dev->page_size - 1) / _mtd_dev->page_size;

uint32_t used_pages = DIV_ROUND_UP(CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ, _mtd_dev->page_size);

benpicco · 2025-01-30T18:06:33Z

sys/fido2/ctap/ctap_mem.c

+        return CTAP1_ERR_OTHER;
+    }
+
+    if (*off > CTAP_FLASH_STATE_SZ && (*off - CTAP_FLASH_STATE_SZ) % CTAP_FLASH_RK_SZ != 0) {


This could need an explanation

Ollrogge requested review from leandrolanzieri, aabadie and MichelRottleuthner as code owners December 25, 2024 16:01

github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Dec 25, 2024

sys/fido2: improve & simplify flash handling

8fdb7a6

Ollrogge force-pushed the fido2_flash_handling_improvements branch from 771229a to 8fdb7a6 Compare December 26, 2024 18:15

mguetschow requested review from mguetschow and benpicco January 24, 2025 14:15

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 28, 2025

mguetschow reviewed Jan 28, 2025

View reviewed changes

fixup! sys/fido2: improve & simplify flash handling

fe73a92

benpicco reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/fido2: improve & simplify flash handling #21110

sys/fido2: improve & simplify flash handling #21110

Ollrogge commented Dec 25, 2024

riot-ci commented Jan 28, 2025 •

edited

Loading

mguetschow left a comment

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

mguetschow Jan 28, 2025

mguetschow Jan 28, 2025

mguetschow Jan 28, 2025

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

benpicco Jan 30, 2025

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

mguetschow Jan 28, 2025

Ollrogge Jan 29, 2025

benpicco Jan 30, 2025

benpicco Jan 30, 2025

benpicco Jan 30, 2025

benpicco Jan 30, 2025

	if (_state.initialized_marker == CTAP_INITIALIZED_MARKER) {
	if (_state.initialized_marker != CTAP_INITIALIZED_MARKER) {


		int ret = mtd_erase(_mtd_dev, addr, sector_size * CONFIG_FIDO2_CTAP_NUM_FLASHPAGES);
		int ret = mtd_erase_sector(_mtd_dev, 0, sector_cnt);

	static mtd_dev_t *_mtd_dev;
	static mtd_dev_t *_mtd_dev = CONFIG_FIDO2_MTD_DEV;

		uint32_t used_pages = (CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ +
		_mtd_dev->page_size - 1) / _mtd_dev->page_size;

	uint32_t used_pages = (CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ +
	_mtd_dev->page_size - 1) / _mtd_dev->page_size;
	uint32_t used_pages = DIV_ROUND_UP(CTAP_FLASH_STATE_SZ + state->rk_amount_stored * CTAP_FLASH_RK_SZ, _mtd_dev->page_size);

sys/fido2: improve & simplify flash handling #21110

Are you sure you want to change the base?

sys/fido2: improve & simplify flash handling #21110

Conversation

Ollrogge commented Dec 25, 2024

Contribution description

Testing procedure

riot-ci commented Jan 28, 2025 • edited Loading

Murdock results

Artifacts

mguetschow 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

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

riot-ci commented Jan 28, 2025 •

edited

Loading