random(4): Gather entropy from Pure sources

[cem]

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

[delphij]

sys/dev/random/random_harvestq.c

251

Why is value set here?

262

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

285

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]

sys/dev/random/random_harvestq.c

251

See the directly following line — this is how the old mask is reported to sysctl.

262

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.

285

The array is not growing in this revision.

I agree about the C99 style array initialization, but that is a totally orthogonal cleanup.

[delphij]

sys/dev/random/random_harvestq.c

251

Oh yes you are right, sorry for the noise.

262

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

285

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]

sys/dev/random/random_harvestq.c

262

Yes, I think your proposed change makes sense. I'll go ahead and make the change.

285

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.

[cem]

Ignore Pure source bits set by user sysctl.

[delphij]

sys/dev/random/random_harvestq.c

285

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.

[cem]

sys/dev/random/random_harvestq.c

285

Good point, I've made that change: https://reviews.freebsd.org/D12618