Stm32wl RNG

[ofauchon]

This is code for using STM32WL's True Random Number Generator:

• PLLQ activation (required by RNG)
• GetRand32() function to get a 32 bit random number.

I added the GetRand32() function in machine package, but I'm not sure this is the good place.
(not even sure it should be part of tinygo core...)

All comments welcome.

[soypat]

I definetly think this should be implemented as a machine.Rand type which provides a partial implementation of the rand APIs in the standard library. I.e

Func (Rand) Uint32() uint32 {

This could then make it easier to develop third party libraries that depend on Randers

[ofauchon]

Hi @soypat,

Thanks for your feedback.
I'll have a look to machine.Rand type and propose a new implementation

[rayozzie]

In the spirit of compatibility with base golang, I believe that this is where TRNG would be implemented.

https://pkg.go.dev/crypto/rand

https://cs.opensource.google/go/go/+/master:src/crypto/rand/

Is the tinygo team's relationship with the main branch team of golang such that they would accept a PR into the main repo for a "machine" build tag?

[soypat]

rayozzie's suggestion seems to be the ideal implementation. Maybe we could have a rand_stm32wl.go file under crypto/rand which assigns the stm32wl reader to crypto/rand.Reader implementation. I'm not sure if it's possible to add single target specific files under stdlib packages without having to copy the whole package.

The way to go about this would be to add a trngreader or similar-named type in said file, much like https://cs.opensource.google/go/go/+/refs/tags/go1.17.3:src/crypto/rand/rand_unix.go. Then implement the Read method for it and assign it to the package level Reader.

[rayozzie]

Honestly, to make things more tractable for the golang team, wouldn’t it make more sense to have a standard interface in the machine package so as to just bother them once? Many stm32 variants and many non-stm32’ s have TRNG’s (mine, for example, is STM32L4R5) and it would seem best IMO to just submit a PR once for all of tinygo and then manage the variants in our world separately. No?

This is all assuming, of course, that they’ll accept a PR for tinygo at all.

[ofauchon]

Hi,

Don't you think I'd be simpler to have a private, tinygo-only Rand implementation.

What would be the benefits of having a full Go crypto/rand.Reader implementation ?

How can we deal with MCUs without RNG devices ?

[rayozzie]

My apologies, but I am not familiar with the overall tinygo philosophy as it relates to 'trying to stay consistent with the base language and core libraries' vs divergence.

It may make sense to do just that. Please interpret my comment as "if the high order bit is to do it the same way as go, here's what probably makes sense".

And to be clear, it's impossible to 'simulate' trng else it would be prng. And so i (personally) would just not implement the machine support for those platforms and let there be a compile error that needs to be dealt with by the dev.

[soypat]

I wasn't talking about bothering the Golang team by asking them to add a rand_stm32wl.go file but rather adding a tinygo implementation of the crypto. In fact, there already is a tinygo implementation you can add to! Check it out:
https://github.com/tinygo-org/tinygo/tree/release/src/crypto/rand
Files you add to the crypto/rand folder are added as part of the standard library for the TinyGo compiler, just like the machine package. So for what it's worth, I think it makes sense to add a stm32-specific crypto.Reader here.

[ofauchon]

I finally :

• implement TRNG driver in src/crypto/rand/rand_stm32wle5.go.
• enable PLLQ clock (required to have TRNG working)

That works fine when testing on nucleo-wl55jc board

[soypat]

LGTM.

[fgsch]

Do you need to do this for every call or this is something that can be done once, for example in the init or Read function?

[kenbell]

Most of the code looks very portable to most stm32. Perhaps put just the initialization code that references AHB3 in a chip-specific file, and the other code for all stm32? @ofauchon if you don’t fancy doing that, I can take a pass after this is merged.

[ofauchon]

@fgsch : You are right ... No need to re-initialize the bus/device at each Read.

So, I moved all the initialization code (AHB3EN bus activation and RNGEN) in the init()

@kenbell : I may not have enough time to make something modular soon, you help will be welcome after the merge.

[aykevl]

Related: #2123 (@zdima)

I'm not entirely happy with putting such low-level chip specific code in the crypto/rand package. I would prefer if low-level code was centralized in the machine and runtime packages. However, in that case it still needs to be used from the crypto/rand package.

So I would propose the following:

1. Add two new functions to the machine package: InitRNG() and ReadRNG() uint32.
2. Use them from the crypto/rand package, using specific build tags (such as // +build stm32wle5 esp32c3 atsamd51). This also means that code to write uint32 values to the output byte slice is not unnecessarily duplicated.

I've picked uint32 (instead of uint8) because that's what many microcontroller families output. I've checked stm32, esp32, nrf, and atsamd51 (nrf is the only one with 8 bit output). Also: if you request random data for cryptographic operations, you probably want a multiple of 4 bytes anyway (128 bits, 256 bits, etc) so there is little loss on a chip that outputs only bytes of random data, for example.

[deadprogram]

To chime in on @aykevl comment: I would like to be able to implement this hypothetical Randomer interface in a driver for the ATECCX08 chip so it can perform the actual random number generation. This would ideally be in the same fashion from "within" crypto/rand package. I do not think that build tags alone can satisfy that particular use case. Thoughts?

[ofauchon]

To chime in on @aykevl comment: I would like to be able to implement this hypothetical Randomer interface in a driver for the ATECCX08 chip so it can perform the actual random number generation. This would ideally be in the same fashion from "within" crypto/rand package. I do not think that build tags alone can satisfy that particular use case. Thoughts?

Why not having a fallback crypto/rand/rand_external.go (When it's not freebsd,linux, windows ...etc).

Then, any machine or external tinygo-driver could 'register' a Randomer interface/functions in rand_external.go

Later, If you try to use crypto/rand and no Randomer is registered -> Read will fire an error.

Sorry If I misunderstood the problem or if it' a bad solution :-)

[deadprogram]

@ofauchon just read your last comment. That sounds like a good strategy to me!

[aykevl]

Actually, it's much simpler. Ron's use case could be solved simply by setting the crypto/rand.Reader variable to another value.

You can easily do something like this:

import "tinygo.org/x/drivers/somedriver"
import "crypto/rand"

func init() {
    rand.Reader = somedriver.New(...)
}

func main() {
    rand.Read(...)
}

Of course that doesn't make it fully magical: you still need to initialize the driver. But I don't think that's a bad thing in this case.
I'm thinking that from a design / layering perspective, using the drivers repo from the main compiler repo is a bad idea. I'd like imports to go just one way: drivers importing packages from the compiler (such as the machine package). The distinction I'm making here is between RNGs provided by the chip and RNGs provided by the board. The former only requires some low level bits, probably provided by the machine package. The latter makes assumptions about the board, and I'd like to avoid such assumptions as much as possible.

@deadprogram would this work for you?

I would like to be able to implement this hypothetical Randomer interface

Nothing hypothetical, it already exists. It is called io.Reader.

[deadprogram]

OK that sounds perfect. Also, I am in total agreement about not consuming drivers from within the compiler packages.

[ofauchon]

Hi all,

Thanks for this gread discussion.
I hope I implemented properly this feature.

Please note :

• I deliberatly named the rand driver "rand_stm32.go", so it can be reused for other STM32 chipsets (not only stm32wl series)
• I added an error in func ReadRNG() (uint32, error) , because TRNG can be unavailable eg: (when not properly clocked), and I think it's important to notice the error to crypto/rand.

Thanks again on time spend on this review .

Olivier

[aykevl]

Looks good to me. However, I think we can name the file rand_baremetal.go instead of rand_stm32.go because there are many other chips with a RNG and they should all use the same machine functions.

[ardnew]

I'm not a big fan of the naming for these 2 functions or the tedious InitRNG as a requirement.

It would be simpler to do lazy initialization on the first call to ReadRNG. This simplifies the requirements on any machine that implements it to a single function call, which I would propose be renamed GetRNG, since it is a fixed-length return variable unlike the various Read functions.

If the machine chooses it wants to add automatic initRNG, it can do so privately in its own init.

[aykevl]

Sounds good to me. It does make reading random numbers slightly slower but that's probably negligible and IMHO outweighs the API simplicity.

No strong opinions on GetRNG vs ReadRNG from me. I picked ReadRNG because it makes it clear that it's not just a getter, but on the other hand you also have getrandom and getentropy syscalls (with a get prefix).

[aykevl]

If the machine chooses it wants to add automatic initRNG, it can do so privately in its own init.

I don't think we should do that because it means the RNG is always initialized, even when unused (which might result in higher current consumption, etc).

[ofauchon]

Hi.

I tried to i include most of your proposals in this last commit :

• Lazy initialisation
• Generic Named Error in machine/machine
• Rename to GetRND

Hope this is the good one :-)

[ofauchon]

The last changes were tested on nucleo-wl55jc board

[deadprogram]

Thank you @ofauchon for your persistence in getting this feature added, and thank you to everyone who helped to define it.

Now merging!