Add an option to use TPM as entropy source

[kd]

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.

[kd]

Spelling in title.

[markm]

LGTM.

[mw]

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]

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

[markm]

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]

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

[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.

2. 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.

[kd]

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]

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]

OK. I like these diffs even better.

[mw]

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().