Page MenuHomeFreeBSD

Remove the Yarrow PRNG algorithm option in accordance with random(4).
ClosedPublic

Authored by markm on Aug 25 2018, 8:23 AM.

Details

Summary

Remove the Yarrow PRNG algorithm option in accordance with random(4).
This includes updating of the relevant man pages, and no-longer-used
harvesting parameters.

PR: 230870

Test Plan

Building the tinderboxes using a kernel with this applied will prove that it works. Docs will need human checking.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 19183
Build 18802: arc lint + arc unit

Event Timeline

markm created this revision.Aug 25 2018, 8:23 AM
markm edited the summary of this revision. (Show Details)Aug 25 2018, 8:30 AM
markm added inline comments.Aug 25 2018, 8:33 AM
UPDATING
34

Yes - I know about this. I'll put the final date in when I get the green light.

markm updated this revision to Diff 47283.Aug 25 2018, 11:28 AM

Add the "differential update" URI to the commit message.
Correct sys/conf/NOTES comment.

bcr added a subscriber: bcr.Aug 25 2018, 7:47 PM

Found a single nit in the man page.

share/man/man4/random.4
162

I think this "The" can also be deleted.

markm updated this revision to Diff 47293.Aug 25 2018, 8:35 PM

Fix documentation nit.

markm marked an inline comment as done.Aug 25 2018, 8:36 PM
markm added inline comments.
share/man/man4/random.4
162

Fixed, thanks!

cem accepted this revision.Aug 25 2018, 8:54 PM

Mostly looks good to me! Some minor doc, style, and one or two functional suggestions below.

UPDATING
36

Maybe note that "some time now" refers to approximately 2003. :-)

share/man/man4/random.4
26

I'm sure you know, but update this on the date you commit it.

156–158

This is an awkward sentence. I would suggest something like (ignoring mdoc formatting):

"random_fortuna is the only algorithm module available."

163

You are allowed to use the full 79-80 characters in manual page lines :-).

343

Ditto -- no need for the really short lines and they make it a bit harder to read.

share/man/man9/random_harvest.9
41

Oh, I did not realize we would also be changing the API, but that makes sense.

sys/conf/files
2824–2826

Can this condition be shortened to just "optional random"? It seems like R || (R && !L) will always just be R.

(And/or, maybe it should instead be "optional random !random_loadable"?)

2826

Ditto

sys/dev/random/random_harvestq.h
43

Can we remove __packed? Given struct harvest_event is used in arrays, it seems like a bad idea to misalign most entries. Alternatively, he_bits could be replaced with an explicit padding byte.

sys/kern/subr_bus.c
2954–2958

This comment is now obsolete.

2958–2959

Is this print really useful? We don't print most harvest sources. Also consider that printf may go to system logs with a timestamp which may reduce the entropy from an attacker's perspective.

2960

The Fortuna authors suggest only harvesting the low bits from timing events since the high bits are fairly predictable.

My understanding is that it doesn't really matter if we're well-seeded with a saved entropy file due to the properties of SHA_d-256, but if we *don't* have that and are still initially seeding we probably don't want to count highly-predictable high-order attach bytes (many of which are likely to be zero, and even the rest may be highly predictable from logs) towards our minimum seed size.

Maybe:

uint8_t attachentropy;

...

attachentropy = attachtime & 0xFF;
random_harvest_direct(&attachentropy, 1, RANDOM_ATTACH);
This revision is now accepted and ready to land.Aug 25 2018, 8:54 PM
markm marked an inline comment as done.Aug 25 2018, 9:56 PM
markm added inline comments.
share/man/man4/random.4
163

I know, but many years ago it was pointed out that breaking sentences into phrases very often made diffs shorter. :-)

It seems that nobody else does that any more so I'll stop too.

sys/conf/files
2824–2826

Damn. I meant to fix those. :-(.
Thanks.

markm updated this revision to Diff 47295.Aug 25 2018, 10:05 PM

Adress some review concerns from cem@, and a few of my own while I'm here.

This revision now requires review to proceed.Aug 25 2018, 10:05 PM
markm marked 12 inline comments as done.Aug 25 2018, 10:09 PM

Address cmem@'s review comments.

cem added a comment.Aug 25 2018, 10:15 PM

Changes mostly look good to me! Most of my concerns were addressed. The only outstanding issue I raised earlier but didn't feel was addressed is the bit around __packed. And just a couple new nits with the new patch below:

sys/dev/random/fortuna.c
247

Why was this changed? I think %hhu was correct and %u isn't. he_size is a uint8_t.

sys/kern/subr_bus.c
2955–2956

style(9) nitpick: comment style looks like this:

/*
 * We only ...
 */

Or maybe:

/* We only ... */

if it all fits in one 80-char line.

But not:

/* We only ...
*/
2956

I think the fact that we only see thousands suggests maybe we should throw away the 9th-16th bits too. But I am ok with this improvement (8 -> 2 bytes) for now.

cem requested changes to this revision.Aug 25 2018, 10:15 PM
This revision now requires changes to proceed.Aug 25 2018, 10:15 PM
markm updated this revision to Diff 47296.Aug 25 2018, 10:20 PM

Rebase to top of HEAD.

markm updated this revision to Diff 47297.Aug 25 2018, 10:39 PM

Sigh. I think I have all cem@'s review comments done now.

markm marked 4 inline comments as done.Aug 25 2018, 10:42 PM
markm added inline comments.
sys/dev/random/fortuna.c
247

Because I'm an idiot! :-( I mean :-)

sys/dev/random/random_harvestq.h
43

Yes we can! Its been a long day...

sys/kern/subr_bus.c
2956

Ranges form tens to thousands have been seen. :-)
This is a *GOOD* entropy source.

markm marked 6 inline comments as done.Aug 25 2018, 10:44 PM
cem accepted this revision.Aug 25 2018, 11:01 PM

Excellent! Again, thanks for taking care of this.

sys/kern/subr_bus.c
2955

Trivial style(9) nit: the first /* should be on a line by itself.

This revision is now accepted and ready to land.Aug 25 2018, 11:01 PM
markm added inline comments.Aug 25 2018, 11:02 PM
share/man/man9/random_harvest.9
28

Reminder to self to fix this.

markm closed this revision.Aug 26 2018, 1:03 PM

Commit r338324 closes this.

markm marked 3 inline comments as done.Aug 26 2018, 1:13 PM