Page MenuHomeFreeBSD

Random bit generator (RBG) driver for RPi and RPi2.
ClosedPublic

Authored by stevek on Jun 18 2016, 10:34 AM.

Details

Summary

This driver supports the following methods to trigger gathering random bits from the hardware:

  1. interrupt when the FIFO is full (default) fed into the harvest queue
  2. callout (when BCM2835_RNG_USE_CALLOUT is defined) every second if hz is less than 100, otherwise hz / 100, feeding the random bits into the harvest queue

If the kernel is booted with verbose enabled, the contents of the registers will be dumped after the RBG is started during the attach routine.

Test Plan

Built RPI2 kernel and booted on board. Tested the different methods to feed the harvest queue (callout, interrupt) and the interrupt driven approach seems best. However, keeping the other method for people to be able to experiment with.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

stevek updated this revision to Diff 17681.Jun 18 2016, 10:34 AM
stevek retitled this revision from to Random bit generator (RBG) driver for RPi and RPi2..
stevek updated this object.
stevek edited the test plan for this revision. (Show Details)
stevek added a reviewer: ARM.
stevek set the repository for this revision to rS FreeBSD src repository.
stevek added subscribers: jtl, sjg.
stevek updated this object.Jun 18 2016, 10:34 AM
markm requested changes to this revision.Jun 18 2016, 11:25 AM
markm edited edge metadata.
markm added inline comments.
sys/arm/broadcom/bcm2835/bcm2835_rng.c
54 ↗(On Diff #17681)

This mode looks like it may be a liability? Should the callout/interrupt-using method not be the default?

73 ↗(On Diff #17681)

There is a mix of tabs and spaces in these defines giving a ragged indentation. Please check style(9) and use only one form for uniformity.

113 ↗(On Diff #17681)

This define looks a little spare here.

127 ↗(On Diff #17681)

An inline function gives slightly better type-safety and better readability, not to mention consistency with the functions below.

165 ↗(On Diff #17681)

Is this right? The general pattern suggests the constant should be 0x1.

Also, can the whole block not be done with some arithmetic, rather than having a bunch of nearly identical constant strings?

247 ↗(On Diff #17681)

It might be a good idea to make this tweekable, like a sysctl, instead of a hard constant.

A panic is quite harsh, so I agree with the #ifdef defaulting to "off".

256 ↗(On Diff #17681)

Yuk :-) A delay here s a no-no. The interrupt/callout method needs to be the default/only method, and this delay needs to go.

426 ↗(On Diff #17681)

Can this also be a sysctl?

436 ↗(On Diff #17681)

Yuk. I know there is precedent for goto's in the BSD kernel, but I'd prefer not to see any new ones. (Preference not insistence).

sys/arm/broadcom/bcm2835/files.bcm283x
13 ↗(On Diff #17681)

"optional random" not "standard" please.

This revision now requires changes to proceed.Jun 18 2016, 11:25 AM

Thanks for this, BTW :-)

stevek added inline comments.Jun 18 2016, 4:50 PM
sys/arm/broadcom/bcm2835/bcm2835_rng.c
54 ↗(On Diff #17681)

Yes, this bled in from my testing.

256 ↗(On Diff #17681)

Okay, I'll just remove the source registration pieces. Makes sense to just axe them since the RBG is not able to keep up with requests as a pure entropy source.

426 ↗(On Diff #17681)

There is a per-device sysctl. See farther below in this function.

stevek updated this revision to Diff 17689.Jun 18 2016, 9:05 PM
stevek edited edge metadata.

I'm not sure what Phabricator is doing with the indentation in bcm2835_rng.c, but it is properly tab indented and the web interface is mutilating the formatting output for the #defines.

The changes in this diff are as follows:
Clean up formatting of some #define(s) to match style(9).
Remove entropy source registration (the RBG cannot keep up, so no sense trying that mode.)
Make the stats incrementing for underruns a static inline function.
Try to detect stalls and reset the device if it happens. If the device stalls twice in one harvest attempt, just disable the device rather than causing potential issues with the rest of the system.
Make the RNG optional, enabling only with the random device is configured.
Add sysctl and tunable for setting stall count.

stevek updated this revision to Diff 17690.Jun 18 2016, 9:11 PM
stevek edited edge metadata.

Updated to eliminate the goto in the attach method.

stevek updated this object.Jun 20 2016, 3:06 PM
stevek edited the test plan for this revision. (Show Details)
jtl added a reviewer: secteam.Jun 20 2016, 3:42 PM
adrian accepted this revision.Jul 19 2016, 12:14 AM
adrian added a reviewer: adrian.
adrian added a subscriber: adrian.

I think this looks pretty good now.

delphij accepted this revision.Jul 19 2016, 7:16 AM
delphij edited edge metadata.

Looks good to me (consider this as a secteam@ approval).

markm added a comment.Jul 19 2016, 4:23 PM

I've got this ready to commit.

We are in code freeze, but I'll see if RE@ buys it, otherwise I'll do it after the freeze is lifted.

brd added a subscriber: brd.Jul 19 2016, 4:42 PM

@markm The freeze has thawed for head (https://www.freebsd.org/releng/), but for MFC RE will need to approve.

This revision was automatically updated to reflect the committed changes.