cryptohome: tpm_wait_ownership can fail right after tpm_take_ownership |
|||
Issue description
What steps will reproduce the problem?
(1) Open developer console on a Chromebook whose TPM isn't owned yet.
(2) Run:
cryptohome --action=tpm_take_ownership
(3) Run (quickly after (2)):
cryptohome --action=tpm_wait_ownership
What is the expected result?
Both commands finish successfully. Running "cryptohome --action=tpm_status" prints "TPM Owned: true".
What happens instead?
The second command (3) sometimes fails, refusing to wait until the TPM ownership is completed.
Our analysis (which may be incomplete or wrong) :
It looks like this may happen because tpm_wait_ownership starts with a synchronous check of the "is_being_owned" flag being true, meanwhile this flag is set by the TPM init task *asynchronously* from a background thread (starting from TpmInit::ThreadMain()). So if the TPM init thread starts its jobs too slowly, then it's possible that the "is_being_owned" flag is still false when the next D-BUS call arrives.
Potential fix, as discussed with apronin@:
Change the command-line cryptohome tool to not check the "is_being_owned" flag. It should wait for the "is_owned" flag.
TODO: Should there still be some diagnostic to catch cases where TPM ownership isn't about to happen?
Workaround:
Instead of using "tpm_wait_ownership", make a loop that spins until "tpm_status" returns "TPM Owned: true".
,
Dec 10
Wei-Cheng do you think your recent changes will have had an impact here?
,
Dec 10
I don't think that's related to my previous change. I think emaxx@ is right. There's racing between the is_being_owned check and the TakeOwnership call since they are handled by 2 different threads. It looks like his suggestion will solve the problem. I will take a look into the issue later.
,
Dec 10
,
Dec 10
Also, on TPM 2.0 we don't even set is_being_owned. So, this test never works. "Wait for ownership" logic in tests was modified to account for that (at least in some cases).
,
Dec 14
I have a WIP CL: crrev/c/1377797, which removes the is_being_owned check. However, it doesn't catch the case that TPM is not owned && taking ownership is not about to happen (e.g., tpm_take_ownership is not called). I have another idea: how about keeping is_being_owned but moving the SetTpmBeingOwned() call from TpmInit::TakeOwnership() into TpmInit::AsyncTakeOwnership() before creating the init thread? We can move the IsTpmEnabled() and IsTpmOwned() checks from TakeOwnership() to AsyncTakeOwnership() and set is_being_owned there. Thus, we can keep current tpm_wait_ownership implementation don't need to worry about the delay. Btw, I think even for 2.0, we still set is_being_owned in TpmInit::TakeOwnership().
,
Dec 14
> how about keeping is_being_owned but moving the SetTpmBeingOwned() call from TpmInit::TakeOwnership() into TpmInit::AsyncTakeOwnership() before creating the init thread? We can move the IsTpmEnabled() and IsTpmOwned() checks from TakeOwnership() to AsyncTakeOwnership() and set is_being_owned there. if the TPM is already owned (or is disabled and can't be owned), we shouldn't set is_being owned. so, yes, AsyncTakeOwnership should check IsOwned & IsEnabled first. and actually don't even bother posting the TakeOwnership task, if TPM is owned already / can't be owned. But that only works if IsOwned and IsEnabled don't talk to the TPM to get the status, but always use the cached status instead. TPM accesses (or talking to tpm_managerd) should happen on MountThread, not on the main dbus thread. So, this change can only happens if cryptohomed requests status from tpm_managerd once only and then relies on signals. > even for 2.0, we still set is_being_owned in TpmInit::TakeOwnership(). yes, looks like we do. it's just we never see it set. since once TakeOwnership task starts, we don't get response to GetStatus until it finishes (in the current setup - refactoring will likely change that) |
|||
►
Sign in to add a comment |
|||
Comment 1 by apronin@chromium.org
, Oct 12