Update FWMP in TPM

chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc

Index: chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
diff --git a/chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc b/chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
index 1a6f976f51e85e99b5fd1a6aa46e96c542faad80..fd9cc39c5daba1bd87b8e068677c2b4629f28f2b 100644
--- a/chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
+++ b/chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
@@ -13,6 +13,9 @@
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/cryptohome/rpc.pb.h"
+#include "chromeos/dbus/cryptohome_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -144,6 +147,17 @@ void AutoEnrollmentController::Start() {
   // auto-enrollment check can start. This happens either after the EULA is
   // accepted, or right after a reboot if the EULA has already been accepted.
 
+  // If a client is being created or already existing, bail out.
+  if (client_start_weak_factory_.HasWeakPtrs() || client_) {
+    LOG(ERROR) << "Auto-enrollment client is already running.";
+    return;
+  }
+
+  // Arm the belts-and-suspenders timer to avoid hangs.
+  safeguard_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(kSafeguardTimeoutSeconds),
+      base::Bind(&AutoEnrollmentController::Timeout, base::Unretained(this)));
+
   // Skip if GAIA is disabled or modulus configuration is not present.
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(chromeos::switches::kDisableGaiaServices) ||
@@ -171,17 +185,6 @@ void AutoEnrollmentController::Start() {
     return;
   }
 
-  // If a client is being created or already existing, bail out.
-  if (client_start_weak_factory_.HasWeakPtrs() || client_) {
-    LOG(ERROR) << "Auto-enrollment client is already running.";
-    return;
-  }
-
-  // Arm the belts-and-suspenders timer to avoid hangs.
-  safeguard_timer_.Start(
-      FROM_HERE, base::TimeDelta::FromSeconds(kSafeguardTimeoutSeconds),
-      base::Bind(&AutoEnrollmentController::Timeout, base::Unretained(this)));
-
   // Start by checking if the device has already been owned.
   UpdateState(policy::AUTO_ENROLLMENT_STATE_PENDING);
   DeviceSettingsService::Get()->GetOwnershipStatusAsync(
@@ -291,14 +294,44 @@ void AutoEnrollmentController::UpdateState(
     case policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR:
     case policy::AUTO_ENROLLMENT_STATE_SERVER_ERROR:
     case policy::AUTO_ENROLLMENT_STATE_TRIGGER_ENROLLMENT:
-    case policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT:
       safeguard_timer_.Stop();
       break;
+    case policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT:
+      RemoveFirmwareManagementParameters();
+      return;

[Thiemo Nagel, 2017/03/23 17:23:52]

Please don't mix return and break in the same switch statement as this is easily
overlooked when reading the code.

[igorcov, 2017/03/24 13:29:15]

Done.

   }
 
   progress_callbacks_.Notify(state_);
 }
 
+void AutoEnrollmentController::RemoveFirmwareManagementParameters() {
+  DCHECK_EQ(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT, state_);
+
+  cryptohome::RemoveFirmwareManagementParametersRequest request;
+  chromeos::DBusThreadManager::Get()
+      ->GetCryptohomeClient()
+      ->RemoveFirmwareManagementParametersInTpm(
+          request,
+          base::Bind(
+              &AutoEnrollmentController::OnFirmwareManagementParametersRemoved,
+              client_start_weak_factory_.GetWeakPtr()));

[Thiemo Nagel, 2017/03/23 17:23:52]

Have you checked the side effects of using client_start_weak_factory_?

+}
+
+void AutoEnrollmentController::OnFirmwareManagementParametersRemoved(
+    chromeos::DBusMethodCallStatus call_status,
+    bool result,
+    const cryptohome::BaseReply& reply) {
+  if (!result) {
+    LOG(ERROR) << "Failed to remove firmware management parameters, error: "
+               << reply.error();
+  }
+
+  if (safeguard_timer_.IsRunning()) {

[Thiemo Nagel, 2017/03/23 17:23:52]

This condition is always true because Timeout() invalidates the weak pointers.

+    safeguard_timer_.Stop();
+    progress_callbacks_.Notify(state_);
+  }
+}
+
 void AutoEnrollmentController::Timeout() {
   // TODO(mnissler): Add UMA to track results of auto-enrollment checks.
   if (client_start_weak_factory_.HasWeakPtrs() &&
@@ -307,15 +340,18 @@ void AutoEnrollmentController::Timeout() {
     // pending, there's a bug in the code running on the device. No use in
     // retrying anything, need to fix that bug.
     LOG(ERROR) << "Failed to start auto-enrollment check, fix the code!";
-    UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
+    state_ = policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT;

[Thiemo Nagel, 2017/03/23 17:23:52]

It seems unclean to update the state manually.  Maybe it would be cleaner to
cancel the timeout before running the FWMP update?  Are you sure we need that
safeguard timer to cover FWMP update?  Is that not covered by a D-Bus timeout?

(Please try to simplify the code.  It's become so complex that it's hard to
convince oneself of its correctness.)

[igorcov, 2017/03/24 13:29:15]

I've removed the FWMP update operation from being covered by safeguard timeout,
as it has its own D-Bus timeout indeed. This allowed for code to be simplified,
thanks.

   } else {
     // This can actually happen in some cases, for example when state key
     // generation is waiting for time sync or the server just doesn't reply and
     // keeps the connection open.
     LOG(ERROR) << "AutoEnrollmentClient didn't complete within time limit.";
-    UpdateState(policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR);
+    state_ = policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR;
   }
 
+  safeguard_timer_.Stop();

[Thiemo Nagel, 2017/03/23 17:23:52]

What's the point in stopping the timer here?  Isn't it stopped already when
Timeout() is called?

+  progress_callbacks_.Notify(state_);
+
   // Reset state.
   Cancel();
 }