cryptohome: move install attributes to mount thread
Let all install attributes operation happen on mount thread to prevent
race condition.
And let install attributes initialize after D-Bus initialized.
BUG=chromium:879797
TEST=FEATURES=test emerge-soraka cryptohome
TEST=tast run $DUT hwsec.Cryptohome*
TEST=Manually enroll, login, logout on soraka
Change-Id: I8615bc6cc6f1857e8516b9ec316965d85e3b27cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2498164
Tested-by: joe Chou <yich@google.com>
Commit-Queue: joe Chou <yich@google.com>
Reviewed-by: Leo Lai <cylai@google.com>
diff --git a/cryptohome/service_userdataauth.cc b/cryptohome/service_userdataauth.cc
index 6105081..0b9c333 100644
--- a/cryptohome/service_userdataauth.cc
+++ b/cryptohome/service_userdataauth.cc
@@ -623,6 +623,20 @@
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesGetReply>> response,
const user_data_auth::InstallAttributesGetRequest& in_request) {
+ service_->PostTaskToMountThread(
+ FROM_HERE,
+ base::BindOnce(&InstallAttributesAdaptor::DoInstallAttributesGet,
+ base::Unretained(this),
+ ThreadSafeDBusMethodResponse<
+ user_data_auth::InstallAttributesGetReply>::
+ MakeThreadSafe(std::move(response)),
+ in_request));
+}
+
+void InstallAttributesAdaptor::DoInstallAttributesGet(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesGetReply>> response,
+ const user_data_auth::InstallAttributesGetRequest& in_request) {
user_data_auth::InstallAttributesGetReply reply;
std::vector<uint8_t> data;
bool result = service_->InstallAttributesGet(in_request.name(), &data);
@@ -639,6 +653,20 @@
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesSetReply>> response,
const user_data_auth::InstallAttributesSetRequest& in_request) {
+ service_->PostTaskToMountThread(
+ FROM_HERE,
+ base::BindOnce(&InstallAttributesAdaptor::DoInstallAttributesSet,
+ base::Unretained(this),
+ ThreadSafeDBusMethodResponse<
+ user_data_auth::InstallAttributesSetReply>::
+ MakeThreadSafe(std::move(response)),
+ in_request));
+}
+
+void InstallAttributesAdaptor::DoInstallAttributesSet(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesSetReply>> response,
+ const user_data_auth::InstallAttributesSetRequest& in_request) {
user_data_auth::InstallAttributesSetReply reply;
std::vector<uint8_t> data(in_request.value().begin(),
in_request.value().end());
@@ -654,6 +682,20 @@
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesFinalizeReply>> response,
const user_data_auth::InstallAttributesFinalizeRequest& in_request) {
+ service_->PostTaskToMountThread(
+ FROM_HERE,
+ base::BindOnce(&InstallAttributesAdaptor::DoInstallAttributesFinalize,
+ base::Unretained(this),
+ ThreadSafeDBusMethodResponse<
+ user_data_auth::InstallAttributesFinalizeReply>::
+ MakeThreadSafe(std::move(response)),
+ in_request));
+}
+
+void InstallAttributesAdaptor::DoInstallAttributesFinalize(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesFinalizeReply>> response,
+ const user_data_auth::InstallAttributesFinalizeRequest& in_request) {
user_data_auth::InstallAttributesFinalizeReply reply;
if (!service_->InstallAttributesFinalize()) {
reply.set_error(
@@ -666,6 +708,20 @@
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesGetStatusReply>> response,
const user_data_auth::InstallAttributesGetStatusRequest& in_request) {
+ service_->PostTaskToMountThread(
+ FROM_HERE,
+ base::BindOnce(&InstallAttributesAdaptor::DoInstallAttributesGetStatus,
+ base::Unretained(this),
+ ThreadSafeDBusMethodResponse<
+ user_data_auth::InstallAttributesGetStatusReply>::
+ MakeThreadSafe(std::move(response)),
+ in_request));
+}
+
+void InstallAttributesAdaptor::DoInstallAttributesGetStatus(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesGetStatusReply>> response,
+ const user_data_auth::InstallAttributesGetStatusRequest& in_request) {
user_data_auth::InstallAttributesGetStatusReply reply;
reply.set_count(service_->InstallAttributesCount());
reply.set_is_secure(service_->InstallAttributesIsSecure());
diff --git a/cryptohome/service_userdataauth.h b/cryptohome/service_userdataauth.h
index a7b9675..02556ca 100644
--- a/cryptohome/service_userdataauth.h
+++ b/cryptohome/service_userdataauth.h
@@ -334,20 +334,36 @@
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesGetReply>> response,
const user_data_auth::InstallAttributesGetRequest& in_request) override;
+ void DoInstallAttributesGet(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesGetReply>> response,
+ const user_data_auth::InstallAttributesGetRequest& in_request);
void InstallAttributesSet(
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesSetReply>> response,
const user_data_auth::InstallAttributesSetRequest& in_request) override;
+ void DoInstallAttributesSet(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesSetReply>> response,
+ const user_data_auth::InstallAttributesSetRequest& in_request);
void InstallAttributesFinalize(
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesFinalizeReply>> response,
const user_data_auth::InstallAttributesFinalizeRequest& in_request)
override;
+ void DoInstallAttributesFinalize(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesFinalizeReply>> response,
+ const user_data_auth::InstallAttributesFinalizeRequest& in_request);
void InstallAttributesGetStatus(
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::InstallAttributesGetStatusReply>> response,
const user_data_auth::InstallAttributesGetStatusRequest& in_request)
override;
+ void DoInstallAttributesGetStatus(
+ std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
+ user_data_auth::InstallAttributesGetStatusReply>> response,
+ const user_data_auth::InstallAttributesGetStatusRequest& in_request);
void GetFirmwareManagementParameters(
std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<
user_data_auth::GetFirmwareManagementParametersReply>> response,
diff --git a/cryptohome/userdataauth.cc b/cryptohome/userdataauth.cc
index 55b4fa0..813b3cc 100644
--- a/cryptohome/userdataauth.cc
+++ b/cryptohome/userdataauth.cc
@@ -44,7 +44,7 @@
namespace cryptohome {
-const char kMountThreadName[] = "MountThread";
+constexpr char kMountThreadName[] = "MountThread";
namespace {
// Some utility functions used by UserDataAuth.
@@ -270,11 +270,6 @@
mount_thread_.StartWithOptions(options);
}
- // If the TPM is unowned or doesn't exist, it's safe for
- // this function to be called again. However, it shouldn't
- // be called across multiple threads in parallel.
- InitializeInstallAttributes();
-
// Clean up any unreferenced mountpoints at startup.
PostTaskToMountThread(FROM_HERE,
base::BindOnce(
@@ -434,6 +429,15 @@
} else {
LOG(ERROR) << __func__ << ": Failed to get TpmManagerUtility singleton!";
}
+
+ // If the TPM is unowned or doesn't exist, it's safe for
+ // this function to be called again. However, it shouldn't
+ // be called across multiple threads in parallel.
+
+ PostTaskToMountThread(FROM_HERE,
+ base::Bind(&UserDataAuth::InitializeInstallAttributes,
+ base::Unretained(this)));
+
PostTaskToMountThread(FROM_HERE,
base::Bind(&UserDataAuth::CreateFingerprintManager,
base::Unretained(this)));
@@ -442,15 +446,15 @@
}
void UserDataAuth::CreateFingerprintManager() {
- if (!default_fingerprint_manager_) {
- default_fingerprint_manager_ = FingerprintManager::Create(
- mount_thread_bus_,
- dbus::ObjectPath(std::string(biod::kBiodServicePath)
- .append(kCrosFpBiometricsManagerRelativePath)));
- }
-
- if (!fingerprint_manager_)
+ if (!fingerprint_manager_) {
+ if (!default_fingerprint_manager_) {
+ default_fingerprint_manager_ = FingerprintManager::Create(
+ mount_thread_bus_,
+ dbus::ObjectPath(std::string(biod::kBiodServicePath)
+ .append(kCrosFpBiometricsManagerRelativePath)));
+ }
fingerprint_manager_ = default_fingerprint_manager_.get();
+ }
}
void UserDataAuth::OnOwnershipTakenSignal() {
@@ -988,7 +992,7 @@
// Initialize the install-time locked attributes since we can't do it prior
// to ownership.
- PostTaskToOriginThread(
+ PostTaskToMountThread(
FROM_HERE, base::BindOnce(&UserDataAuth::InitializeInstallAttributes,
base::Unretained(this)));
@@ -1013,7 +1017,7 @@
}
void UserDataAuth::DetectEnterpriseOwnership() {
- AssertOnOriginThread();
+ AssertOnMountThread();
static const std::string true_str = "true";
brillo::Blob true_value(true_str.begin(), true_str.end());
@@ -1022,9 +1026,7 @@
brillo::Blob value;
if (install_attrs_->Get("enterprise.owned", &value) && value == true_value) {
// Update any active mounts with the state, have to be done on mount thread.
- PostTaskToMountThread(FROM_HERE,
- base::BindOnce(&UserDataAuth::SetEnterpriseOwned,
- base::Unretained(this), true));
+ SetEnterpriseOwned(true);
}
// Note: Right now there's no way to convert an enterprise owned machine to a
// non-enterprise owned machine without clearing the TPM, so we don't try
@@ -1032,7 +1034,7 @@
}
void UserDataAuth::InitializeInstallAttributes() {
- AssertOnOriginThread();
+ AssertOnMountThread();
// Don't reinitialize when install attributes are valid.
if (install_attrs_->status() == InstallAttributes::Status::kValid) {
@@ -2712,27 +2714,33 @@
bool UserDataAuth::InstallAttributesGet(const std::string& name,
std::vector<uint8_t>* data_out) {
+ AssertOnMountThread();
return install_attrs_->Get(name, data_out);
}
bool UserDataAuth::InstallAttributesSet(const std::string& name,
const std::vector<uint8_t>& data) {
+ AssertOnMountThread();
return install_attrs_->Set(name, data);
}
bool UserDataAuth::InstallAttributesFinalize() {
+ AssertOnMountThread();
return install_attrs_->Finalize();
}
int UserDataAuth::InstallAttributesCount() {
+ AssertOnMountThread();
return install_attrs_->Count();
}
bool UserDataAuth::InstallAttributesIsSecure() {
+ AssertOnMountThread();
return install_attrs_->is_secure();
}
InstallAttributes::Status UserDataAuth::InstallAttributesGetStatus() {
+ AssertOnMountThread();
return install_attrs_->status();
}
diff --git a/cryptohome/userdataauth_unittest.cc b/cryptohome/userdataauth_unittest.cc
index c7c528f..5d11caa 100644
--- a/cryptohome/userdataauth_unittest.cc
+++ b/cryptohome/userdataauth_unittest.cc
@@ -807,6 +807,8 @@
EXPECT_CALL(attrs_, Get("enterprise.owned", _))
.WillOnce(DoAll(SetArgPointee<1>(blob_true), Return(true)));
userdataauth_->Initialize();
+ userdataauth_->set_dbus(bus_);
+ userdataauth_->PostDBusInitialize();
EXPECT_TRUE(userdataauth_->IsEnterpriseOwned());
}
@@ -821,6 +823,8 @@
EXPECT_CALL(attrs_, Get("enterprise.owned", _))
.WillOnce(DoAll(SetArgPointee<1>(blob_true), Return(true)));
userdataauth_->Initialize();
+ userdataauth_->set_dbus(bus_);
+ userdataauth_->PostDBusInitialize();
EXPECT_FALSE(userdataauth_->IsEnterpriseOwned());
}
@@ -3048,6 +3052,12 @@
ASSERT_TRUE(userdataauth->Initialize());
},
base::Unretained(userdataauth_.get())));
+ userdataauth_->set_dbus(bus_);
+ PostToOriginAndBlock(base::BindOnce(
+ [](UserDataAuth* userdataauth) {
+ ASSERT_TRUE(userdataauth->PostDBusInitialize());
+ },
+ base::Unretained(userdataauth_.get())));
}
protected:
