random: add hwrng and puf_sram as FEATURES_OPTIONAL

[fjmolinas]

Contribution description

This PR adds periph_hwrng and puf_sram as FEATURES_OPTIONAL for random module.

Right now if auto_init is used random seed is initiated from puf_sram, periph_hwrng, periph_cpuid or RANDOM_SEED_DEFAULT

RIOT/sys/random/random.c

Lines 42 to 61 in 999fffd

	void auto_init_random(void)
	{
	uint32_t seed;
	#ifdef MODULE_PUF_SRAM
	/* TODO: hand state to application? */
	if (puf_sram_state) {
	LOG_WARNING("random: PUF SEED not fresh\n");
	}
	seed = puf_sram_seed;
	#elif defined (MODULE_PERIPH_HWRNG)
	hwrng_read(&seed, 4);
	#elif defined (MODULE_PERIPH_CPUID)
	luid_get(&seed, 4);
	#else
	LOG_WARNING("random: NO SEED AVAILABLE!\n");
	seed = RANDOM_SEED_DEFAULT;
	#endif
	DEBUG("random: using seed value %u\n", (unsigned)seed);
	random_init(seed);
	}

.

But pub_sram is never compiled which means cpu/boards as samr21-xpro end up using there CPU_ID instead of a random seed. So adding it as a FEATURES_OPTIONAL allows it to be included in the build if available.

Testing procedure

• Compile tests/puf_sram on a board supporting puf_sram:

make -C tests/puf_sram BOARD=samr21-xpro flash test

• Enable DEBUG in random.c flash an application using random on BOARD providing puf_sram and periph_hwrng. In the first case the seed should change everytime the board is power-cycled, on the second case every-time the board is reset.

make -C examples/gcoap/ BOARD=samr21-xpro flash term

make: Entering directory '/home/francisco/workspace/RIOT/examples/gcoap'
/home/francisco/workspace/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "115200"
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2019-09-04 09:13:41,526 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2019-09-04 09:13:47,659 - INFO # random: PUF SEED not fresh
2019-09-04 09:13:47,662 - INFO # random: using seed value 973387205
2019-09-04 09:13:47,670 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:13:47,671 - INFO # gcoap example app
2019-09-04 09:13:47,674 - INFO # All up, running the shell now
> 2019-09-04 09:13:52,725 - INFO #  random: PUF SEED not fresh
2019-09-04 09:13:52,729 - INFO # random: using seed value 2016792513
2019-09-04 09:13:52,737 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:13:52,739 - INFO # gcoap example app
2019-09-04 09:13:52,741 - INFO # All up, running the shell now
> 2019-09-04 09:14:01,152 - INFO #  random: using seed value 2902826692
2019-09-04 09:14:01,160 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:14:01,162 - INFO # gcoap example app
2019-09-04 09:14:01,164 - INFO # All up, running the shell now

make -C examples/gcoap/ BOARD=pba-d-01-kw2x flash -j3 term

2019-09-04 09:09:14,641 - INFO # random: using seed value 3994478280
2019-09-04 09:09:14,650 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:09:14,652 - INFO # gcoap example app
2019-09-04 09:09:14,654 - INFO # All up, running the shell now
> 2019-09-04 09:09:16,023 - INFO #  random: using seed value 3830827191
2019-09-04 09:09:16,031 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:09:16,033 - INFO # gcoap example app
2019-09-04 09:09:16,036 - INFO # All up, running the shell now
> 2019-09-04 09:09:17,053 - INFO #  random: using seed value 920084802
2019-09-04 09:09:17,062 - INFO # main(): This is RIOT! (Version: 2019.07-devel-754-ge5de1-HEAD)
2019-09-04 09:09:17,063 - INFO # gcoap example app
2019-09-04 09:09:17,065 - INFO # All up, running the shell now

Issues/PRs references

[fjmolinas]

@cladmi does this violate any of the concepts behind the dependency parsing refactor?

[cladmi]

What behavior is this supposed to give? I understand the description but do not get what behavior is wanted.

From a reading review of the changes and the previous code only:

I think this changed the behavior in some cases.
Because if a board provides both puf_sram and periph_hwrng they will be both included.
Where before, if you defined USEMODULE += puf_sram in your application you would not have periph_hwrng (note that it is an ifeq(,$(filter puf_sram,$(USEMODULE)) so gets inside when the module is not here to handle default behavior)

The previous behavior however had determinism issues though, as if puf_sram was added as a sub dependency after multiple iterations of the parsing, periph_hwrng would be already in features optional, but not if puf_sram was added as dependency in the application Makefile.

So needs some careful look of what is wanted.
And depending on this, how we can achieve it.

[fjmolinas]

What behavior is this supposed to give? I understand the description but do not get what behavior is wanted.

You are right I didn't present enough of a description. I complemented the PR description accordingly.

Because if a board provides both puf_sram and periph_hwrng they will be both included.
Where before, if you defined USEMODULE += puf_sram in your application you would not have periph_hwrng (note that it is an ifeq(,$(filter puf_sram,$(USEMODULE)) so gets inside when the module is not here to handle default behavior)

You are right. I changed this because even though the code gets built it wont end up in the compiled fw, since the initialization for random_init prioritizes it over puf_sram.

I could revert that specific change and remove periph_hwrng from USEMODULE if puf_sram and random are present in USE_MODULE, would that be ok? Would you suggest a cleaner way?

[fjmolinas]

@cladmi ping!

[cladmi]

What I understand:

The current handling is that, puf_sram is only included when requested explicitly. There may be a good reason that @PeterKietzmann would know.

What you want, is use puf_sram by default if possible.
Now using hwrng is done if possible, if puf_sram was not included.

Which makes you want to implement I guess, for random, "use puf_sram in priority", "otherwise hwrng", and otherwise the backup. Am I right?

The current implementation has the limitation that this choice will be done by the C code, and if both are available, both puf_sram and hwrng_init would be called and so compiled and linked.
It is a consequence of the decision that "init" is done in auto_init and not in the module including it.
It could be acceptable if documented properly.

Doing the dependency resolution you want with the current dependency system is not standard. It could be done by looking into FEATURES_PROVIDED, if done with a function would keep limited mis-uses.
FEATURES_OPTIONAL += $(call find_me_the_first_of_these_provided_features, puf_sram periph_hwrng)

[fjmolinas]

The current handling is that, puf_sram is only included when requested explicitly. There may be a good reason that @PeterKietzmann would know.

I will wait for @PeterKietzmann input then. I'll try to get a proof of concept for using LPM to initialize puf_sram and that way it could be used more widely.

Which makes you want to implement I guess, for random, "use puf_sram in priority", "otherwise hwrng", and otherwise the backup. Am I right?

Yes which is replicating the explicit hierarchy specified in code.

Doing the dependency resolution you want with the current dependency system is not standard. It could be done by looking into FEATURES_PROVIDED, if done with a function would keep limited mis-uses.
FEATURES_OPTIONAL += $(call find_me_the_first_of_these_provided_features, puf_sram periph_hwrng)

I will look into this. Thanks for the suggestion.

It is a consequence of the decision that "init" is done in auto_init and not in the module including it.
It could be acceptable if documented properly.

OK!

[fjmolinas]

@cladmi would this be ok then?:

find_first_of_these_provided_features = $(firstword $(filter $1,$(FEATURES_PROVIDED)))

FEATURES_OPTIONAL += $(call find_first_of_these_provided_features, puf_sram periph_hwrng)

I have this result

make -C examples/gcoap/ BOARD=samr21-xpro info-build

FEATURES_USED:
         periph_cpuid periph_gpio periph_gpio_irq periph_pm periph_spi periph_timer periph_uart puf_sram
FEATURES_REQUIRED:
         periph_gpio periph_gpio_irq periph_spi periph_timer periph_uart
FEATURES_OPTIONAL_ONLY (optional that are not required, strictly "nice to have"):
         periph_cpuid periph_pm puf_sram
FEATURES_OPTIONAL_MISSING (missing optional features):
         -none-
FEATURES_PROVIDED (by the board or USEMODULE'd drivers):
         cpp cpu_check_address periph_adc periph_cpuid periph_flashpage periph_flashpage_raw periph_flashpage_rwee periph_gpio periph_gpio_irq periph_i2c periph_pm periph_pwm periph_rtc periph_rtt periph_spi periph_timer periph_uart periph_uart_modecfg periph_usbdev puf_sram riotboot

[PeterKietzmann]

The current handling is that, puf_sram is only included when requested explicitly. There may be a good reason that @PeterKietzmann would know.

As @fjmolinas pointed out, the puf-sram seeder requires a cold reboot in order to provide entropy. Now that (I assume) many developers are working with a dev kit connected via USB, I wanted to avoid using this as a default.

[fjmolinas]

@PeterKietzmann @maribu I think now that #13137 its is safe to reactivate this one right?

[fjmolinas]

I implemented the hack suggested by @cladmi while we do not have way of handling multiple optional features.

[maribu]

For some non-cryptographic use cases the PUF SRAM based seed should now be viable even with soft reboots. The remaining issue is that the hash after soft reboots will not look very random (e.g. the first byte will only change after cold boots). And the theoretical possibility of devices being tracked when the seed is used in an observable way. (However, I'm not a cryptographer and don't have the background to estimate how practical such an attack would be. So it could be that this would be super trival, or totally infeasible as an 32 bit hash might just as well leak to little information about the SRAM characteristics to be of use. Or anything in between.)

The use of a hash with better pre-image resistance would however solve both issues, but would increase overhead of the boot process significantly and increase ROM requirements. The CPU overhead could be mitigated to some degree by reducing the size of the SRAM chunk to hash to the smallest size that contains enough entropy (plus safety margin). But some research would be required to identify that size. Maybe @PeterKietzmann is already working on this (or similar)? If so, I'd wait for his result getting to RIOT first.

[fjmolinas]

@maribu sounds good!

[miri64]

Ping @maribu @PeterKietzmann?

[maribu]

I think one would prefer periph_hwrng over puf_sram (at least currently), as periph_hwrng has less impact on boot up time and (at least in case of warm boots) more entropy.

Edit: I meant in case both a present, periph_hwrng should IMO be preferred of puf_sram.

[PeterKietzmann]

Agreed

[fjmolinas]

What I was doing here resembles the FEATURES_REQUIRED_ONE_OUT_OF but FEATURES_OPTIONAL_ONE_OUT_OF, I think I should probably re-work it that way.

[fjmolinas]

Can I then just invert the ordering here?

[maribu]

Yep :-)

[fjmolinas]

Rebased.

[fjmolinas]

@maribu since what I was doing was similar to your ONE-OUT-OFF approach, I decided to put it in common and use the same approach here.

[fjmolinas]

@maribu since what I was doing was similar to your ONE-OUT-OFF approach, I decided to put it in common and use the same approach here.

Note that FEATURES_OPTIONAL_ALT declares alternatives, but does not express possible conflicts, it just expresses a preference between multiple nice-to-have features.

[benpicco]

Needs a rebase.
That FEATURES_OPTIONAL_ALT is certainly nice, but I don't know enough about to build system to judge if this can cause issues.

Btw.: with my sleepy_node test I needed to enable puf_sram as otherwise it would always start with the same sequence number and the CoAP server would ignore later requests as duplicates.
Using puf_sram was enough to get different random numbers after a warm boot.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.