random: added a random driver interface

[mehlis]

how should this be designed?

I see this PR as a discussion forum for this long standing discussion

[LudwigKnuepfer]

How about random_get_uint32 ?

[LudwigKnuepfer]

How are other systems implementing interfacing this?

I would have gone for bytes (unsigned char random_get(void)) or generic size (random_fill(unsigned char *ptr, size_t amount)).

[LudwigKnuepfer]

Regarding *arch:
It should be possible to use several sources of randomness, hardware sources among them. I think it might make sense to specify driver classes (weak, strong, ...) so the boards can decide which ones they implement.

In any case this is something where we should look how other systems are doing it as well.

[mehlis]

@haukepetersen can you review?

[haukepetersen]

Just did. I think the interface looks well, just two remarks:

1. I would like to change the return values to int and short, as all the other low-level drivers build on these types
2. @LudwigOrtmann a method for getting a number of random bytes would be helpful, but could have timing side-effects. When using hardware-peripherals, they need time to generate the numbers. E.g. from the Atmel SAM3X datasheet:

The True Random Number Generator (TRNG) passes the American NIST Special Publication
800-22 and Diehard Random Tests Suites.
As soon as the TRNG is enabled (TRNG_CTRL register), the generator provides one 32-bit
value every 84 clock cycles

So getting a (larger) number of bytes could take significant time. This we need either to well document or leave this to a higher-level interface...

[LudwigKnuepfer]

@haukepetersen What do you suggest? Should the driver add TRNG values to a pool on its own or should there be something like a bool trng_data_available()?

[LudwigKnuepfer]

I agree that a higher level driver should provide blocking access to slow randomness.

[haukepetersen]

I would leave the interface 'as is'. For retrieving one 32-bit value waiting 84 cycles is ok (and other platforms may have significantly different numbers here -> e.g. 40 cycles on the STM32F4), as long as its documented.

I would strongly argue against a pool or similar on the low-level driver layer, as I think the low level layer should be kept as slim as possible.

[LudwigKnuepfer]

Well, the driver could feed to an upper driver via callback..

[haukepetersen]

Ok, I did a quit shot at an implementation for the SAM3X -> #1294, turned out to be quite simple :-)

[mehlis]

@haukepetersen what about an API like:

int random_get(void *buf, int bytes);

the driver tries to fill bytes bytes in buf. it will return the actual number of random bytes (driver can choose to only provide less bytes of randomness)

@thomaseichinger I think this is what you suggested some weeks ago, right?

[haukepetersen]

@mehlis I think this interface would work well, I would go with this. Any other opinions on this?

[OlegHahm]

2.1

[OlegHahm]

I think uint8_t and uint32_t as return values are fine.

Regarding the initialization: wouldn't it make indeed sense to give a list of sources as parameters to this function?

[thomaseichinger]

Well, the prototype mentioned in #939 (comment) I proposed with some future asynchronous functionality in mind especially for platforms needing more than <100 cycles and applications needing more than 4 bytes. Also it would result in one single function to feed a definable range of bytes. But I don't have a very strong opinion on this.

[haukepetersen]

I spend some further thought on this, maybe we can have an interface like the following, that provides synchronous as well as asynchronous access (similar to the UART interface):

int random_get_blocking(*buf, num_of_bytes);   // will block until random data is available
int random_get_async(void (*cb)(uint data));      // gets new bytes whenever ready
void random_done();    // will stop generating random data

Using this interface, it should be easy to put a higher-level driver on top, that can asynchronously generate random number of any length and synchronize via messaging...

This IF must surely be though over a little more, but what do you think about the general direction?

[thomaseichinger]

I like the general idea. But I maybe cb should get a parameter to know where to store the data, so it can work without a global buffer. Something like

/* gets new bytes whenever ready and stores in buf */
int random_get_async(void (*cb)(uint data, uint *buf, uint *read_bytes), 
                                    uint *buf, 
                                    uint *read_bytes); 

But cb would in this case need to keep state to know which consecutive byte it reads and where to store.
Or would this be the duty of a higher-level driver because it gets quite complex then again?

[haukepetersen]

Nope, I think we missunderstood each other. The callback gets intentionally only one parameter - the newly created random value. It is then up to the higher-level driver/consumer to do something with that value, e.g. to store it into some kind of buffer...

[mehlis]

@haukepetersen I updated the interface with your input, please review

[Kijewski]

values > 0 I presume

[haukepetersen]

agreed with @Kijewski

[thomaseichinger]

@mehlis please change this

[Kijewski]

The number of bytes should be unsigned. Return 0 on error.

[mehlis]

@haukepetersen updated with your new input

[thomaseichinger]

I don't get this. Is returning 1 meant to signal that more random data is needed?

[mehlis]

@thomaseichinger yes, in case the callback returns 1, the driver is informed to call it again, if 0 is returned the driver will unregister the callback

[mehlis]

@haukepetersen updated with your input

[tfar]

I'd suggest a blocking API, similar to that @mehlis suggested. I do not see much use in a callback interface. Basically your application gets high quality entropy at start and initializes its CSRNG and can generate random numbers at a fast pace.

Ideally the API would be similar to already known and existing APIs, e.g. OpenBSD's getentropy() [1] or soon Linux's getrandom() [2]. So basically like the already suggested int random_get(void *buf, int bytes);, just with proper types.

[1] http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2?&manpath=OpenBSD-current&arch=amd64&query=getentropy
[2] http://lists.openwall.net/linux-kernel/2014/07/17/235

[OlegHahm]

Is this still WIP and what's the state of this PR?

[haukepetersen]

Yes, still WIP, but I think as soon as @thomaseichinger is back, we should be able to merge this rather soon.

[miri64]

Since @haukepetersen was quite nitpicky about putting net_dev (see #1492) into the low-level driver interface (and I think he was right) I would like to point out that (since most implementations would propably use the boards ADC) might not really be low-level ;-)

[haukepetersen]

@authmillenon I like to disagree here, I think most implementations would actually be using hardware peripherals as (Pseudo) random number generators to implement this interface, and therefore I would see it in the low-level drivers.

[LudwigKnuepfer]

@authmillenon I'd also say that the fact that a peripheral might make use of another peripheral driver does not in itself suffice to say it's not a low level driver. Or I don't understand the argument at all.

[miri64]

Okay as I was saying: I just wanted to point out my understanding of this. But if you all think differently, then go on :-)

[LudwigKnuepfer]

We are the iBorg.

[haukepetersen]

Ok, given some more thought, I think the original proposed interface should be fine:

int random_init(void);
int random_get(char *buffer, int length);

The documentations should state, that you have to take a careful look at the comments on the implementation, which tell you details about (i) the timing behavior of the driver and (ii) the grade of randomness for the specific implementation.

[mehlis]

@haukepetersen updated, in case you want more changes, it's probably best you take care of this interface (by doing an updated PR)

[haukepetersen]

you can remove this line, unsigned int < 0 is very rare anyhow... :-)

[Kijewski]

Well, 0 is still an invalid input.

[haukepetersen]

True. Ignore my comment above.

[haukepetersen]

@mehlis I created a PR against your branch

[thomaseichinger]

Looks good to me, ACK

[haukepetersen]

and go.