-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert system to CONFIG_RNG/CSRNG with automatic configuration of ENTROPY_GENERATOR #83022
Conversation
Removal of TinyCrypt makes mbedtls the default when CSPRNG_ENABLED is set. No longer need the assignments in the respective defconfigs. Signed-off-by: David Leach <[email protected]>
@valeriosetti here is my pass at what I've been discussing. Probably easier to just look at this at the commit level (first one is a cleanup that I should have removed). |
drivers/entropy/Kconfig
Outdated
help | ||
Include entropy drivers in system config. | ||
|
||
if ENTROPY_GENERATOR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this was necessary due to circular dependency problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried to do something similar in #82497 @nordicjm told me that this was not correct. In my case I was auto-enabling CONFIG_ENTROPY_GENERATOR
whenever "zephyr,entropy" chosen property was set, while here we are completely removing that Kconfig, but I think the conclusion is similar: there would be no "high level" Kconfig symbol which is gating the sub-Kconfigs for the platform specific driver.
@nordicjm wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a top level Kconfig controlling all sub-configs, you should not see or be able to configure e.g. the log level of a feature if the feature is not enabled so removing this is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we say that but the we have a annoying dependency tree between ENTROPY_GENERATOR and enabling random. I'm going to leave this like it is for now while we discuss the larger objective here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weirdness we have in the system is that IF you want to access the rand/csrand functions the only way to do this is to enable ENTROPY_GENERATOR, which is also the enablement for ENTROPY and entropy_get_entropy(). We also made this overall configuration such that the application needs to know to set this configuration if it happens to include some subsystem/module component that needs rand or entropy. I want to get the subsystems and modules that know they need rand and/or entropy to automatically set the appropriate Kconfig values instead of moving that responsibility to the application layer configuration.
The only thing I want the application layer configuration to have to set is the TEST_RANDOM_GENERATOR to make an overt decision to use something that is not really a valid generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weirdness we have in the system is that IF you want to access the rand/csrand functions the only way to do this is to enable ENTROPY_GENERATOR, which is also the enablement for ENTROPY and entropy_get_entropy().
Let me add to this that enabling ENTROPY_GENERATOR
is the only way to poll if there is any way to really have some entropy driver enabled for that platform or not. Ideally one would first try to enable that, see what happens, and then decide whether they can use it or need to rely on some fake/test generator.
Of course this is not possible because parsing of Kconfig is done in a single pass, so that's why I came up with CSPRNG_AVAILABLE
in #82859.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth looking at other systems and selections in zephyr that do this, for example the C library selection - there is minimal, picolibc, newlib and external, there is a Kconfig that applications (and systems) can select to say that they require a full C library, that is, not the minimal C library:
config REQUIRES_FULL_LIBC
bool "Require complete C library"
help
Select a C library implementation that provides a complete C library
implementation, rather than the subset provided by MINIMAL_LIBC.
This is selected by the following options:
SIMPLELINK_HOST_DRIVER
LIBLC3
IEEE802154_KW41Z
NPM1300_CHARGER
REQUIRES_FULL_LIBCPP
POSIX_DEVICE_IO
LORAWAN
UPDATEHUB
Then there is an option which specifies if there is a full C library available:
config FULL_LIBC_SUPPORTED
bool
help
Selected when the target has at least one C library that offers a
complete implementation and which would be selected when
REQUIRES_FULL_LIBC is set.
Which is selected by the following:
NEWLIB_LIBC_SUPPORTED
PICOLIBC_SUPPORTED
NATIVE_LIBC
Then there is another Kconfig which selects which C library you have active, because whilst it might default to picolibc, perhaps you want to use newlib. That sort of design and implementation should be reused for entropy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the circular dependency is the ENTROPY_HAS_DRIVER. Looking at the examples you highlight to see how to work around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the hint @valeriosetti suggested and now have a ENTROPY_AVAILABLE config driven from DT node. This helps break the dependency loops with HAS_DRIVER.
Not really network specific change so reassigning. |
config RNG | ||
default y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot do this, this will completely bypass dependencies. As an example, take a board that does not have support for rng, e.g. niosv_g
, configure hello world for it, run menuconfig, go to RNG and confirm that the option cannot be selected:
config RNG
bool "Enable inclusion of RNG functions"
default y
imply ENTROPY_GENERATOR(=n)
depends on ENTROPY_HAS_DRIVER(=n) || TEST_RANDOM_GENERATOR(=n)
help
Subsystems and applications that need to use the sys_rand_get() set
of APIs will set this config.
ENTROPY_HAS_DRIVER is true if the DT is enabled.
Now go and enable networking and ethernet, this now selects RNG even though the option's requirements have not been met
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it wont turn on. If you don't have entropy and you haven't turned on TEST_RANDOM_GENERATOR then you can't use RNG.
TEST_RANDOM_GENERATOR has to be set for this to be set if you don't have an entropy driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the same thing but it kind of operates as expected. The actual selection of the RNG/CSRNG doesn't show up until you select the "Allow non-random number generator". When you do then you can select RNG/CSRNG. If you unselect the non-random selection, the RNG/CSRNG goes away.
I guess the question here though is if that makes sense. Since the target doesn't support a true entropy source it still presents the option to select the non-random source.
Add ENTROPY_AVAILABLE Kconfig to be set when DT zephyr,entropy node is set. This will be used for dependency in other modules to avoid circular dependency on ENTROPY_HAS_DRIVER. Signed-off-by: David Leach <[email protected]>
1b669db
to
8a26fed
Compare
Add RNG and CSRNG configuration options for the system and application layer to set when they want to use RNG/CSRNG APIs. Remove CSPRNG_ENABLE from Kconfig and replace usage in tree with CSRNG. Signed-off-by: David Leach <[email protected]>
Automatically enable ENTROPY_GENERATOR when RNG and/or CSRNG is true. The system used to require the explicit setting of ENTROPY_GENERATE to enable the random APIs. Now the system (subsystems, drivers, applications, etc) will set RNG/CSRNG based on the area's known need for random API instead of turning on something in Entropy. ENTROPY_GENERATE still can be set directly if the subsystem/driver/etc needs access to the entropy APIs. Signed-off-by: David Leach <[email protected]>
Convert the internal Kconfig from ENTROPY_GENERATOR to RNG. Signed-off-by: David Leach <[email protected]>
Convert subsystom components configuration from ENTROPY_GENERATOR to RNG/CSRNG Signed-off-by: David Leach <[email protected]>
Convert drivers to use new RNG/CSRNG configuration where needed. Signed-off-by: David Leach <[email protected]>
Kconfig configuration for mbedtls and openthread enable RNG/CSRNG configuration for the system. Signed-off-by: David Leach <[email protected]>
The new RNG/CSRNG configuration will automatically support setting the right Kconfig values. Signed-off-by: David Leach <[email protected]>
Convert over to automatic configuration of ENTROPY_GENERATOR. Only use CONFIG_RNG when the sample is directly accessing the sys_rand API functions. Use CONFIG_ENTROPY_GENERATOR when the sample is directly accessing the entropy API. Signed-off-by: David Leach <[email protected]>
Convert tests that directly use sys_rand API to use CONFIG_RNG instead of CONFIG_ENTROPY_GENERATE. Signed-off-by: David Leach <[email protected]>
8a26fed
to
27c818f
Compare
Now that @valeriosetti PR has merged, I'm going to close this and reattempt later doing a RNG/CSRNG configuration |
Have explicit configuration of entropy and RNG/CSRNG by the modules, subsystems, drivers, and kernel when needed.
Introduce a new RNG and CSRNG Kconfig in the subsys/random to manage automatic configuration of ENTROPY_GENERATOR.
Applications will only explicitly configure RNG, CSRNG, and/or ENTROPY_GENERATOR when the application code calls sys_rand/csrand APIs or the entropy_get_entropy API.