Page MenuHomeFreeBSD

Support for bcm2838 RNG
AcceptedPublic

Authored by me_jamesrm.com on Nov 22 2019, 12:11 AM.

Details

Summary

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.

Test Plan

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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Please upload diff with full context.

delphij added a subscriber: delphij.

Refresh diff with svn head.

Looks mostly fine to me.

sys/arm/broadcom/bcm2835/bcm2835_rng.c
116

Please make this static.

130

Ditto.

434

Maybe assert sc->conf != NULL here instead? (the probe method already returned ENXIO).

LGTM, modulo delphij's comments.

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.

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
334–337

Original?

356–358

Correct.

443–444

I mean if you aren’t the original author, do you even have the rights to submit this patch? Can you get in contact with the original author?

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.

Updated per review, sent to me by James Mintram <me@jamesrm.com>.

James Mintram <me@jamesrm.com> reworked the patch I wrote after some email discussion. Submitting his updates on top.

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.

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
147

This probably needs to be a const pointer, or I doubt this will compile.

316

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.)

318

sometimes spelled MIN(cnt, chunk_size) (sys/param.h).

322

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.

454

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
322

Oh, great catch! Thanks.

James Mintram <me@jamesrm.com> reworked the patch I wrote after some email discussion. Submitting his updates on top.

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.

Fixes pointer arithmetic and adds const qualifier to conf data pointers.

Please upload diffs with full context — either using the arcanist tool, or with diff -U999999 (The 9s are just a stand-in for infinity).

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 *

This revision is now accepted and ready to land.Jun 22 2020, 5:22 PM