random(4): Gather entropy from Pure sources
ClosedPublic
Actions

Authored by cem on Oct 6 2017, 5:32 PM.

Details

Reviewers

jmg
delphij

Group Reviewers

O3: Kernel Random Numbers Generator
secteam

Commits

rS324394: random(4): Gather entropy from Pure sources

Summary

At initialization, hc_source_mask only includes non-Pure sources.

The patch changes source registration to enable the registered source in the
hc_source_mask bitmask. This mask governs which sources are harvested.

This patch also disallows userspace from disabling such sources.

PR: 222807
Submitted by: W. Dean Freeman <badfilemagic AT gmail.com>
Obtained from: HBSD 0054e3e170e083811acc9f3b637f8be8a86c03e7
Security: yes

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

cem created this revision.Oct 6 2017, 5:32 PM

delphij added inline comments.

sys/dev/random/random_harvestq.c
251 ↗	(On Diff #33764)	Why is value set here?
262 ↗	(On Diff #33764)	Looks like user can set harvest mask bit (rather than not being permitted to modify, therefore not match the comments) -- is this intentional? Or should this be something like: (value & orig_value) \| (orig_value & RANDOM_HARVEST_PURE_MASK)?
284 ↗	(On Diff #33764)	Since this is growing, will it be sensible to move this to the header file with the enum itself? (also probably a good idea to define random_source_descr as random_source_descr[RANDOM_ENTROPYSOURCE]. Another suggestion is to use C99 style array initialization, e.g. [RANDOM_CACHED] = "CACHED".

cem added inline comments.Oct 6 2017, 6:14 PM

sys/dev/random/random_harvestq.c
251 ↗	(On Diff #33764)	See the directly following line — this is how the old mask is reported to sysctl.
262 ↗	(On Diff #33764)	I don't understand the confusion. The user is allowed to freely modify the bits of the mask corresponding to environmental sources. They are only disallowed from modifying the bits corresponding to Pure sources. Yes, it is intentional that they can set or clear bits not present in RANDOM_HARVEST_PURE_MASK.
284 ↗	(On Diff #33764)	The array is not growing in this revision. I agree about the C99 style array initialization, but that is a totally orthogonal cleanup.

sys/dev/random/random_harvestq.c
251 ↗	(On Diff #33764)	Oh yes you are right, sorry for the noise.
262 ↗	(On Diff #33764)	So should the code be changed to: harvest_context.hc_source_mask = (value & ~RANDOM_HARVEST_PURE_MASK) \| (orig_value & RANDOM_HARVEST_PURE_MASK); ? Otherwise the user would be able to set an unset bit within RANDOM_HARVEST_PURE_MASK (but not clearing it)?
284 ↗	(On Diff #33764)	There is no description for RANDOM_PURE_VIRTIO and RANDOM_PURE_BROADCOM, should they be added? (I think the array should still be extended to ENTROPYSOURCE, by the way). I agree that the cleanup being orthogonal to this change.

cem marked an inline comment as done.Oct 6 2017, 6:35 PM

cem added inline comments.

sys/dev/random/random_harvestq.c
262 ↗	(On Diff #33764)	Yes, I think your proposed change makes sense. I'll go ahead and make the change.
284 ↗	(On Diff #33764)	I guess they should be added, but I am not very familiar with how this array is used. It can be done in a subsequent revision.

Ignore Pure source bits set by user sysctl.

sys/dev/random/random_harvestq.c
284 ↗	(On Diff #33764)	Well, its usage is in line 327 (previously 299). Because you have changed the loop from ENVIRONMENTAL_END to ENTROPYSOURCE, potentially the loop would access undefined area, and at minimum the array should be extended to ENTROPYSOURCE.