cryptohome/tpm2 performance: optimize tpm readiness/status checks |
||||||
Issue descriptioncryptohomed currently requests full tpm status, which leads to re-sending GetCapability from tpm_managerd each time it needs to check if tpm is ready. This can lead on waiting on completing long tpm commands and in general is a waste of cpu time. Optimize by implementing some subset (or full set) of: 1) For tpm readiness: hecking locally if tpm readiness file is present. If not, tpm is not ready. Change the order of the checks. 2) For tpm ownership when not owned: request the status from the tpm_managerd once and cache it until cryptohomed tells the tpm_manager to take ownership. until then nobody should take the ownership anyways. 3) Add a signal that the tpm manager sends when the tpm is fully initialized (owned). Re-request the tpm state only upon receiving such signal. 4) Implement a targetd request to tpm_managerd to check only the owned/initialized state. This way tpm-manager will know when a full status is requested and when only some part of capabilities can be re-read.
,
Jan 13 2018
Also depends on tpm initialization refactoring: partially landed https://crrev.com/c/698805, partially WIP https://crrev.com/c/720212.
,
Aug 24
,
Aug 24
Commenting on the candidate CL https://crrev.com/c/1187963/1 Short-term it's probably a good fix to disentangle cryptohomed from tpm_managerd in those checks (and help with isssues like issue 801748 now). Unf, longer-term it's not that easy. If is_owned == false you need to keep checking. 1) The TPM doesn't have to become owned immediately after calling Tpm2Impl::TakeOwnership, so setting it after that call may become wrong in the near future. Iirc this call is blocking now, but it shouldn't be (since initialization is a long process), one of the next steps is to change it to become non-blocking. 2) tpm_managerd has a mode when it auto-owns the TPM w/o any signal from other daemons. So, it's possible cryptohomed starts while the TPM is not owned, but then it becomes owned w/o cryptohomed doing anything. Also, technically, anybody can send TakeOwnership to tpm_managerd, cryptohomed shouldn't rely on it. What I'd do is 1) Implement a new signal that tpm_managerd sends once it owns the TPM (similar to what we want to do in issue 256845 but for Owned event, not AttestationPrepared event). 2) Possibly, implement a new tpm_managerd message - IsOwned that quickly checks the cached flag, possibly w/o even going to the worker thread to avoid waiting for previous long operations. 3) Make cryptohomed read the 'owned' status once on start. And only re-read it (actually, no need to do that even) when it gets the 'Ownership is taken' message from tpm_managerd. As further steps (separate CLs) we need to change tpm_managerd and its callers (that's part of issue 777688): (a) create a non-blocking version of TakeOwnership that just requests it but doesn't wait for it to end; use it in cryptohomed, use the signal from above to kick off dependent steps. (b) Switch to using async versions of trunks calls inside tpm_managerd. And break a single TakeOwnership task on worker thread into separate steps, where the next step is launched when a response to a TPM command is received. (That should be done for other tasks as well, but TakeOwnership is the most important.) That is done to avoid blocking requests that don't need communicating to the TPM (such as reading the cached status w/o refreshing) while waiting for a result of a long command.
,
Aug 27
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/959e0e394704d52919f85812c5cc6d63dcb4c14e commit 959e0e394704d52919f85812c5cc6d63dcb4c14e Author: Wei-Cheng Xiao <garryxiao@chromium.org> Date: Fri Sep 28 02:45:02 2018 tpm_manager: Add an ownership taken signal to tmp2 initializer Also fixed build errors in dbus_service_test.cc but kept the test excluded (crbug/624437). Added unit tests to dbus_service_test.cc, in which tests should be checked again when people work on crbug/624437 later. BUG=chromium:777679 TEST=ran unit tests against soraka, chell, and gale; manually tested on a soraka device Change-Id: I66494a2bc5e66eaa9b14d22cb1c38d4803d3ef63 Reviewed-on: https://chromium-review.googlesource.com/1195284 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Wei-Cheng Xiao <garryxiao@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/dbus_service_test.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_status_impl.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_manager_service.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_initializer_impl.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_status_impl.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_status_test.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/common/tpm_ownership_dbus_interface.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_status.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/dbus_service.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_manager_service.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/mock_tpm_status.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_initializer_test.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_util.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_initializer_impl.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_status_impl.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/main.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm2_initializer_impl.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_status_impl.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/mock_tpm_status.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_initializer_impl.cc [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/dbus_service.h [modify] https://crrev.com/959e0e394704d52919f85812c5cc6d63dcb4c14e/tpm_manager/server/tpm_manager_service_test.cc
,
Sep 29
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/d353c9c6d810e8757683a3a1c4d68e27edfac942 commit d353c9c6d810e8757683a3a1c4d68e27edfac942 Author: Wei-Cheng Xiao <garryxiao@chromium.org> Date: Sat Sep 29 07:27:32 2018 cryptohome: add ownership taken signal receiving and ownership state caching. We only need to add caching for the TPM 2.0 part. TPM 1.2 has its own caching and doesn't have the problem of repeating expensive TPM status inquiry. BUG=chromium:777679 TEST=unit tests on soraka, chell, and gale; manually tested on soraka and chell devices Change-Id: I3e084e00d57ac8cd10175832e25fb5e9e86fbe33 Reviewed-on: https://chromium-review.googlesource.com/1220416 Commit-Ready: Wei-Cheng Xiao <garryxiao@chromium.org> Tested-by: Wei-Cheng Xiao <garryxiao@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service_distributed.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm2_impl.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm_impl.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service_monolithic.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service_monolithic.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm2_impl.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm_impl.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/stub_tpm.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/mock_tpm.h [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service_distributed.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm2_test.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/service.cc [modify] https://crrev.com/d353c9c6d810e8757683a3a1c4d68e27edfac942/cryptohome/tpm.h
,
Oct 12
,
Nov 2
,
Nov 28
,
Nov 29
The only thing left in this bug is optimizing the performance of TPM readiness check. Currently TPM is ready iff all the following conditions are true: 1. TPM is enabled, 2. TPM is owned, 3. TPM is not being owned, and 4. a "tpm-is-owned" file (/mnt/stateful_partition/.tpm_owned) exists. To check 3 and 4, we don't need to send any dbus request from cryptohome. The "being owned" status is only created and used by cryptohome internally. For 4, I am even not sure if we need that or not, although we don't need worry about that here. The 2nd check has been done via signaling and caching, although cryptohome still needs to request for TPM status from tpm_managerd once. Therefore, the only thing we need to optimize is checking if TPM is enabled, and we can apply the same signaling and caching logic to the check. However, we don't want to ask tpm_managerd for TPM status twice, which is expensive, when doing both the "is-enabled" and "is-owned" checks. I plan to add two more dbus methods to tpm_managerd: IsTpmEnabled and IsTpmOwned, which return the cached status, and have the client daemons call those 2 dbus methods instead. Also, when the first time tpm_managerd sees that the TPM is enabled (probably during initialization), it emits a signal, just like when TPM is owned. Both cryptohomed and attestationd will subscribe to the "is-enabled" and "is-owned" signals (cryptohomed already subscribes to "is-owned") and cache/update the statuses internally.
,
Nov 29
> I plan to add two more dbus methods to tpm_managerd: IsTpmEnabled and IsTpmOwned, which return the cached status, and have the client daemons call those 2 dbus methods instead. Can we combine all checks in a single dbus method that returns cached status for owned + enabled? > For 4, I am even not sure if we need that or not, although we don't need worry about that here. If I remember correctly, this was the indication that TPM is actually initialized, not just "owned" as reported from In case with tpm_managerd (currently only on 2.0, but will be extended to 1.2), both .tpm_owned file and "is being owned" should go away (in fact, 2.0 doesn't ever set "is being owned" already). > when the first time tpm_managerd sees that the TPM is enabled (probably during initialization), it emits a signal 'Enabled' is const for ChromeOS daemons: For 2.0, the TPM is always enabled. There's no disabled state for it. (Currently, tpm_managerd for 2.0 treats it a bit differently: enabled means PH is disabled, other are enabled; but that's not required; besides, even if we keep doing this, this state also doesn't change). For 1.2, there is such state. But nobody does anything with it in Chrome OS (only firmware and recovery image switch from disabled to enabled if needed), so all we need to do is detect if TPM is enabled/disabled at start, and then it never changes. If it is disabled, tpm_managerd and the rest can just treat it as if there is no TPM at all. For 1.2, on all modern devices, this info is available through "/sys/class/**/tpm0/device/enabled" sysfs entries. I suggest, we just rely on those. (Though it's ok to keep the current fallback of sending GetCapability to read permanent flags and get 'disable' bool from there) tpm_managerd can read it on start, other daemons just ask it (since they need to ask for 'owned' info anyways). But no need for a signal there.
,
Dec 3
I found cryptohome and attestation (platform2/attestation) have slightly different definitions of TPM readiness. To summarize: For 1.2, cryptohome - check 1) isEnabled, 2) isOwned, 3) isBeingOwned, and 4) if file .tpm_owned exists. All those information is obtained locally (without needing to send any dbus request to other daemon). 3 and 4 should go away eventually. For 1.2, attestation (platform2/attestation) - check 1) /sys/class/.../enabled and 2) /sys/class/.../owned. Again, no dbus request here at all. For 2.0, cryptohome - check 1) isEnabled, 2) isOwned, 3) isBeingOwned, and 4) if file .tpm_owned exists. 1 and 2 should come from tpm_managerd although can be cached if the value is true. Again, 3 and 4 are obtained locally and should go away eventually. For 2.0, attestation - check 1) isEnabled, 2) isOwned, 3) if DA lockout is NOT in effect. All should come from tpm_managerd. We can combine owned + enabled + da_lockout check in a single dbus method in TpmOwnershipInterface, and tpm_managerd can return cached results if possible. Now the questions are 1. Do we need to DA lockout check in attestationd? Should that be consistent with cryptohomed? 2. You said for 2.0, there is no disabled state for the TPM. If PH is not disabled (i.e., in recovery mode), should we still treat the TPM as enabled? Or to simplify, should tpm_managerd always say TPM is enabled (2.0 only)? If that's true, other daemons even don't need to ask tpm_managerd for it.
,
Dec 4
> 1. Do we need to DA lockout check in attestationd? Should that be consistent with cryptohomed? For "is TPM ready" purposes, attestationd doesn't need to check for DA lockout. In general, PrepareForEnrollment or Enroll may want to consider DA lockout status: trying to use EK that requires authorization would fail anyways while in lockout. But trying shouldn't harm either: auth requests, correct or not, are just auto-rejected w/o consequences in lockout. And checking this state requires an extra trip to the TPM (GetCapability) each time we want to do that. So, I'd say, you are right, let's indeed not check for DA lockout in attestationd. > 2. You said for 2.0, there is no disabled state for the TPM. If PH is not disabled (i.e., in recovery mode), should we still treat the TPM as enabled? Or to simplify, should tpm_managerd always say TPM is enabled (2.0 only)? If that's true, other daemons even don't need to ask tpm_managerd for it. Yes, we can still treat TPM is enabled if PH is still enabled. tpm_managerd can always set .enabled to true for 2.0. Given that cryptohomed/attestationd need to call tpm_managerd once for status anyways, I'd suggest cryptohomed/attestationd just take .enabled from that response and cache it - it never changes during cryptohomed/attestationd lifetime. And leave it up to tpm_managerd how it determines if it's enabled or not. This way we'll have a single point where it is determined, and we can change in any way we like in the future. All that is true both for 1.2 and 2.0, especially with 1.2 also switching to having a separate tpm_managerd in the future.
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/36ce314d759fa1308b27124042d5d59afa0abf96 commit 36ce314d759fa1308b27124042d5d59afa0abf96 Author: Wei-Cheng Xiao <garryxiao@chromium.org> Date: Wed Dec 05 09:13:31 2018 tpm_manager: add readiness-only support to the dbus msg GetTpmStatus Also have TpmManagerService always say TPM is enabled (2.0 only). (See https://bugs.chromium.org/p/chromium/issues/detail?id=777679#c14) Client daemons will use this new feature to check if a TPM is enabled and owned, two essential conditions for TPM readiness check. This avoids sending the GetCapacity command to TPM, which is expensive, when the client only cares about TPM readiness. Client side changes will be in future CLs. BUG=chromium:777679 TEST=unit test, $ tpm_manager_client status --readiness_only Message Reply: [tpm_manager.GetTpmStatusReply] { status: STATUS_SUCCESS enabled: true owned: true } Change-Id: I09b8284de6a7ec7c66c6bd023c9a9a257cf94f72 Reviewed-on: https://chromium-review.googlesource.com/1359636 Commit-Ready: Wei-Cheng Xiao <garryxiao@chromium.org> Tested-by: Wei-Cheng Xiao <garryxiao@chromium.org> Reviewed-by: Andrey Pronin <apronin@chromium.org> [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/common/tpm_manager.proto [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/common/print_tpm_manager_proto.cc [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/common/tpm_ownership_interface.h [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/server/tpm_manager_service.cc [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/server/tpm2_status_test.cc [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/server/tpm2_status_impl.cc [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/server/tpm_manager_service_test.cc [modify] https://crrev.com/36ce314d759fa1308b27124042d5d59afa0abf96/tpm_manager/client/main.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by apronin@chromium.org
, Jan 13 2018