tpm2: Avoid attempting to open duplicate TPM connections #360
Conversation
chrisccoulson
force-pushed
the
tpm2-avoid-duplicate-connections
branch
from
610b686 to
2249d34
Compare
When using NewTPMPassphraseProtectedKey, it is passed an already open connection. But the core secboot package calls back into the platform handler to set the user auth value, which opens a second connection. When using the direct device (/dev/tpm0), this doesn't work. This updates the test suite to make sure that this scenario is caught and results in an error. It also fixes the problem by providing the open TPM connection to the singleton tpm2 platform handler so that it uses this rather than trying to open a new connection. This also identified a bug that the core secboot package was not passing the role value to the platform handler, which only seemed to be caught by adding tests that created passphrase protected TPM keys with a role.
chrisccoulson
force-pushed
the
tpm2-avoid-duplicate-connections
branch
from
2249d34 to
963b7b7
Compare
chrisccoulson
changed the title
tpm2/seal.go
Outdated
| // Avoid trying to reopen another connection when setting the user auth value | ||
| orig := mainPlatformKeyDataHandler.tpm | ||
| mainPlatformKeyDataHandler.tpm = tpm | ||
| defer func() { mainPlatformKeyDataHandler.tpm = orig }() |
There was a problem hiding this comment.
depending how things are used this could create a data race, do we need a global mutex around some of the handler usage?
There was a problem hiding this comment.
Yes, that might be a good idea - I'll add that. We might want to actually hold the lock throughout the whole code section that runs with the connection set, so that any other code that wants to set the connection will have to wait, else things might get a bit confusing if there are multiple connections around.
There was a problem hiding this comment.
Whilst trying to figure out the best way to do this, and given that I'd like to reduce existing data races, I ended up taking a different approach by permitting the open connection to be passed to NewKeyDataWithPassphrase and subsequently back to the tpm2 platform handler via a new context argument to ChangeAuthKey. It seemed like the safest way to do it in the end.
Rather than setting a Connection on the platform handler on the tpm2 side and then the calling PlatformKeyDataHandler.ChangeAuthKey for the first time, try permitting the Connection to be passed directly to ChangeAuthKey instead. This adds a new parameter that can be of any type to KeyWithPassphraseParams. This is passed directly to PlatformKeyDataHandler.ChangeAuthKey when it is called for the first time. Plarforms are free to use this new parameter however they like, but the tpm2 package uses it to pass an open TPM connection. When called via KeyData.ChangePassphrase, nothing is passed via the new argument and the tpm2 package will open a new connection. This avoids race conditions with the previous approach and introducing new thread safety issues at a time when I'd like to be reducing these overall.
When using
NewTPMPassphraseProtectedKey, it is passed an already openconnection. But the core secboot package calls back into the platform
handler to set the user auth value, which opens a second connection.
When using the direct device (/dev/tpm0), this doesn't work.
This updates the test suite to make sure that this scenario is caught
and results in an error. It also fixes the problem by providing a way to
pass the open TPM connection to the singleton tpm2 platform handler
via a new field on the
secboot.KeyWithPassphraseParamsstruct whichcan be set to any value and is passed to the
ChangeAuthKeyimplementationwhen setting the initial auth value. The tpm2 platform handler
implementation uses this connection if passed rather than trying to open a
new connection. If the
ChangeAuthKeyimplementation is called viaKeyData.ChangePassphrase, then the new argument will be nil and thetpm2 implementation will open a new TPM connection as required.
This also identified a bug that the core secboot package was not passing
the role value to the platform handler, which only seemed to be caught
by adding tests that created passphrase protected TPM keys with a role.
This addresses issue #353