Page MenuHomeFreeBSD

Add an option to use TPM as entropy source
ClosedPublic

Authored by mindal_semihalf.com on Mar 18 2019, 9:39 AM.

Details

Summary

TPM has a builtin RNG, with its own entropy source.
You can read more about requirements RNG has to satisfy here.
Driver was extended to harvest 16 random bytes every 10 seconds and pass them to system entropy pool.
A new built option was added to enable this feature, as it is disabled by default.
Also use this opportunity to clean up indentation a little bit.

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

mindal_semihalf.com retitled this revision from Add an option to TPM as entropy source to Add an option to use TPM as entropy source.Mar 18 2019, 9:39 AM

Spelling in title.

markm accepted this revision as: markm.Mar 21 2019, 1:00 PM
markm added a subscriber: markm.

LGTM.

This revision is now accepted and ready to land.Mar 21 2019, 1:00 PM
mw added a comment.Mar 21 2019, 1:55 PM

Hi @markm ,

I got following message when attempting to commit:

Committing transaction...
svn: E165001: Commit failed (details follow):
svn: E165001: Commit blocked by pre-commit hook (exit code 1) with output:

  • You need "Approved by: (security-officer|so|secteam|core)" line in your log entry.

Is your approval sufficient to give one of the missing "Approved by:"? Or should we ask someone else to additionally approve?

Thanks,
Marcin

emaste added a subscriber: emaste.Mar 21 2019, 2:20 PM

As a general comment, if we're going to make whitespace changes they should be done separately, first.

markm added a comment.Mar 21 2019, 3:00 PM

Hi

You need the security officer's say-so. I have a personal wavier when I do such commits myself (as I wrote the main random device), but this is non-transferrable, as far as I know.

M

gordon added a subscriber: gordon.Mar 21 2019, 6:09 PM

Strong agree on separating any whitespace commits from functional commits. This is especially important for security sensitive content.

gordon requested changes to this revision.Mar 21 2019, 6:10 PM
This revision now requires changes to proceed.Mar 21 2019, 6:10 PM
delphij requested changes to this revision.Mar 21 2019, 7:31 PM
delphij added a subscriber: delphij.

I have noticed a few minor issues and have commented inline.

sys/dev/tpm/tpm20.c
36 ↗(On Diff #55184)

Could you please add a comment here to explain why 10 seconds was chosen? (It's not consistent across the tree: some drivers choose 0.1 seconds, while some others uses 4 seconds, 5 seconds, etc.)

Optional suggestions:

  1. If I was you, I would probably use a "ticks" number here instead of seconds (e.g. (10 * hz)). It's not immediately obvious what unit was used here, and it would help clearing the ambiguity that way.
  1. Another idea is to use a field in tpm_sc to store a pre-calculated number. The current code would do the multiplication every time the callout_reset is invoked.
189 ↗(On Diff #55184)

Here we are scheduling a callout after 1 tick (0 is silently converted to 1 here), should we wait for TPM_HARVEST_INTERVAL for the first invoke?

Note that because we are scheduling a callout here, we should perform a callout_drain() on device detach.

271 ↗(On Diff #55184)

If I understood the code correctly, does this mean we would no longer have future tpm20_harvest invokes if we ever get a zero-sized response from TPM? If so, is it intentional?

(I think we should still schedule it, or in other words, something like:

if (entropy_size > 0) {
	entropy_size = MIN(entropy_size, TPM_HARVEST_SIZE);
	memcpy(entropy, sc->buf + TPM_HEADER_SIZE + sizeof(uint16_t), entropy_size);
}

sx_xunlock(&sc->dev_lock);

if (entropy_size > 0) {
	random_harvest_queue(entropy, entropy_size, RANDOM_PURE_TPM);
}

callout_reset(&sc->harvest_callout, hz * TPM_HARVEST_INTERVAL,
		tpm20_harvest, sc);

)

Note that line 259 is similar but I'm not sure if a failed sc->transmit counts as a fatal error.

sys/dev/tpm/tpm_crb.c
37 ↗(On Diff #55184)

It's worthy to do this and tpm_tis.c change in a separate commit.

mindal_semihalf.com marked an inline comment as done.Mar 22 2019, 10:30 AM

Remove style changes, they will be committed separately. Also update per feedback.

sys/dev/tpm/tpm20.c
36 ↗(On Diff #55184)

Thanks for the feedback.

Could you please add a comment here to explain why 10 seconds was chosen? (It's not consistent across the tree: some
drivers choose 0.1 seconds, while some others uses 4 seconds, 5 seconds, etc.)

I've added a comment. To expand on that basically discrete TPMs are painfully slow. I wanted to avoid a situation where the chip is used so often by the harvest command that it has an impact on userspace applications. Because of that the I'd prefer the interval to be in an order of a few seconds.

189 ↗(On Diff #55184)

Here we are scheduling a callout after 1 tick (0 is silently converted to 1 here), should we wait for TPM_HARVEST_INTERVAL for the first invoke?

This is intentional. I wanted to fire it as soon as we are ready. IMHO it doesn't make any sense to wait for a rather long amount of time before the first harvest is performed.

Note that because we are scheduling a callout here, we should perform a callout_drain() on device detach.

Whoops. You're right obviously. Fixed it.

271 ↗(On Diff #55184)

This was intentional. I considered a zero-size response as a some kind of fatal error. Frankly I'm fine with trying again in that case, so I changed it.

sys/dev/tpm/tpm_crb.c
37 ↗(On Diff #55184)

Done

delphij accepted this revision.Mar 22 2019, 4:48 PM

Thanks! LGTM (note that discard_buffer_callout should probably also be drained, but it's unrelated to this change).

sys/dev/tpm/tpm20.c
217 ↗(On Diff #55344)

Maybe also discard_buffer_callout here? (Not really related to your change and therefore optional).

markm accepted this revision.Mar 22 2019, 4:55 PM

OK. I like these diffs even better.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 23 2019, 5:14 AM
This revision was automatically updated to reflect the committed changes.
mw added a comment.Mar 23 2019, 5:25 AM

Hi, just to avoid unnecessary hassle - the only minor improvement I made when commiting, was to add missing "#ifdef TPM_HARVEST" around callout_drain in tpm20_release().