The hardware RNG on the Raspberry Pi 4 differs slightly from the version found in the Pi 3. This patch extends the existing bcm2835_rng.c driver to function on the Pi4. However, I do not expose all of the register debug facilities that are available on the pi 3 driver.
Details
Details
Tested on my Pi 4, the numbers given appear random, but I have not had time to generate a statistical analysis of the collected numbers. @kevans stated he would check the driver still functions correctly on a Pi 3.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 28609 Build 26643: arc lint + arc unit
Event Timeline
Comment Actions
No real objections from me either; I'm unfamiliar with the hardware and the random(4) integration seems mostly good. Some nits and suggestions below.
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 116 | And perhaps const | |
| 249 | Should the RND_WARM_CNT mask be part of sc->conf? | |
| 327–331 | I'm not sure this is ever what we want? | |
| 334–337 | Is this something that can happen, and do we have reason to believe this procedure will fix the RNG? Is it possible the RNG might continue producing output after we've observed zero bytes available? I.e., we're just consuming it too fast but it'll continue to produce entropy bytes at some rate? | |
| 356–358 | I'd suggest wrapping this in a subroutine that submits multiple random_harvest_queue() invocations with at most HARVESTSIZE * sizeof(uint32_t) (i.e., 8) bytes per invocation. Otherwise, the high quality random data will be squashed together into at most 8 bytes, which is a pretty big waste if we're pulling e.g. 32 bytes from this device. Maybe random(4) should provide an API for high quality sources instead, I don't know. (Or, uh, sizeof(((struct harvest_event *)0)->he_entropy), but that's kind of a big ugly expression.) | |
| 432 | Maybe just (void *), especially if it unwraps the line. | |
| 434 | +1 | |
| 443–444 | Should we default to 2x mode (but allow it to be disabled with the tunable? Is there some reason to believe 2x mode will be a worse default (higher power draw, perhaps)? | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 249 | I'd be tempted to make it part of sc->conf and s/!sc->conf->can_diagnose/sc->conf->rnd_warm_cnt_mask != 0/ up above, removing can_diagnose. | |
Comment Actions
Thanks for reviewing this.
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 334–337 | I don't know, that was the original algorithm. I haven't seen it in action. | |
| 356–358 | Are you saying that random_harvest_queue() only takes some of the random data supplied? If I use the API to submit 10 MB of random data, it effectively only extracts 8 bytes of randomness? | |
| 443–444 | I don't know, you'd have to ask the original author. It doesn't seem to exist on the newer 2838 chip, at least, as far as I could figure out. I agree the toggle seems like a complication that probably offers zero value. | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 443–444 | I honestly did not realize that only the original author of a file was entitled to submit a patch. I'm happy to withdraw this if that's the case. | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 443–444 | I think conrad meant original author of the patch... | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 443–444 | I wrote this patch, which edits the existing BCM2835 driver from mainline FreeBSD, to add support for the BCM2838 chip. I did not write that original driver. This patch does not intend to affect the existing behaviour of the "bcmrng.2xspeed" mode on the BCM2835 chip. The "2x speed" mode seems like a weird quirk, but I don't want to comment authoritatively on whether it should exist or not, nor whether it should be default enabled. The "2x speed" mode does not appear to exist on the BCM2838 chip, which is why I moved it under an if-block in this patch. | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 356–358 | To be clear, it is a bad API. I'm not defending it, just telling you where the foot-gun is so you can avoid it. | |
| 443–444 | (It's a red herring here but: yes, I meant the original author of the patch, not the file. I believe you may be able to submit patches on behalf of others; you obviously need at least their permission and license of copyright.) I was confused when you referred me to the "original author" for questions about what I perceived as changes you had authored. Your latest comment makes clear that the revision is just selectively disabling the existing behavior on the new chip, which doesn't have that functionality. That's perfectly fine! (Probably I should have noticed that when I was first reading the change — my bad.) It also would have been a less confusing answer to the original (confused) question. Thanks. | |
Comment Actions
James Mintram <me@jamesrm.com> reworked the patch I wrote after some email discussion. Submitting his updates on top.
Comment Actions
If a committer is needed, I'm happy to oblige, but I'll need testing time, and for that I need to build and boot. If Someone Else(™) can assure me that this works, I'll speed it up a bit.
Comment Actions
Mostly looks good to me. A few style nits below. One thing that definitely must be fixed (pointer arith error). Thanks!
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 149 | This probably needs to be a const pointer, or I doubt this will compile. | |
| 317 | Probably makes sense to use uint32_t for size, too, given it is assigned either cnt or chunk_size (both uint32_t). (Yes, these are the same size on all FreeBSD archs, but I think it is best to be consistent.) | |
| 319 | sometimes spelled MIN(cnt, chunk_size) (sys/param.h). | |
| 323 | This math is wrong. Due to pointer arithmetic rules, this will increment sc_buf_chunk by a factor of sizeof(uint32_t) more than you intend. I would suggest just using char *sc_buf_chunk and perhaps initializing it with a (void *)sc->sc_buf cast. | |
| 433 | I think the message text might be less confusing if it said "sc->conf == NULL". Not a big deal. | |
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 323 | Oh, great catch! Thanks. | |
Comment Actions
There is an option to commandeer a review under the "Add Action" drop down James can use if Robert is happy for him to take over the updates.
Comment Actions
Please upload diffs with full context — either using the arcanist tool, or with diff -U999999 (The 9s are just a stand-in for infinity).
Comment Actions
LGTM. A few minor style nits.
One thing it might be nice to do before commit is to grab some raw output from the generator and run it through a tester. PractRand is pretty fast (on server-class hardware, not Rpi) and should be fed as much data as you can stand (I don't know how fast the random source is, but "many GBs" is an acceptable amount of input to PractRand, if it doesn't take too long to gather). The runtime of PractRand itself is something like a few minutes for GB-length inputs.
NIST has their SP800-90B Entropy estimator: https://github.com/usnistgov/SP800-90B_EntropyAssessment . This test is very slow (you'd want to run it on a fast amd64 machine, not on rpi directly, and it takes several hours) but gives some estimate of min-entropy. For this one, I'd only feed it about 1 MB of RNG output.
| sys/arm/broadcom/bcm2835/bcm2835_rng.c | ||
|---|---|---|
| 78 | Usually we put const before the underlying type name (const struct bcm_rng_conf *). No functional difference. Style(9) also wants a space before the *. | |
| 175 | style(9) nit: space between void and * | |
| 234 | Style nit: const void * | |