Update FWMP in TPM

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #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"
 
 namespace chromeos {
 
 namespace {
 
 // Maximum time to wait before forcing a decision.  Note that download time for
 // state key buckets can be non-negligible, especially on 2G connections.
@@ skipping 268 matching lines @@
     case policy::AUTO_ENROLLMENT_STATE_PENDING:
       break;
     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;
   }
 
+  if (state_ == policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT) {
+    StartRemoveFirmwareManagementParameters();
+  } else {
+    progress_callbacks_.Notify(state_);
+  }
+}
+
+void AutoEnrollmentController::StartRemoveFirmwareManagementParameters() {
+  DCHECK_EQ(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT, state_);
+
+  cryptohome::RemoveFirmwareManagementParametersRequest request;
+  chromeos::DBusThreadManager::Get()
+      ->GetCryptohomeClient()
+      ->RemoveFirmwareManagementParametersFromTpm(
+          request,
+          base::Bind(
+              &AutoEnrollmentController::OnFirmwareManagementParametersRemoved,
+              base::Unretained(this)));

[Thiemo Nagel, 2017/03/27 17:21:47]

base::Unretained() seems unsafe.  Are you sure this usage is correct?

[igorcov, 2017/03/28 16:39:09]

Not sure if it counts as "check", but safeguard_timer_ used it since forever.
And the reason it was introduced is the fact that the flow is blocked until
progress_callbacks_ are notified.

I've checked in the code, this object lives inside login_display_host_impl and
there's a chain leading from notification being sent to the impl_ object but it
is linked through some weak_ptr in binds, so I couldn't find a way to prove it
theoretically.

[Thiemo Nagel, 2017/03/29 10:47:21]

I think unless specified in the contract for the class (i.e. the class-level
comment), it should be possible to destroy the class at any time without causing
crashes due to pending callbacks.  Thus imho the use of Unretained for
safeguard_timer_ was wrong from the start.

As a quick fix, you could add a (second) weak_ptr_factory_.  In the long run,
I'd like to merge the two, but that's maybe best done in a separate CL.

[igorcov, 2017/03/29 15:17:17]

Done.

+}
+
+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();
+  }
+
   progress_callbacks_.Notify(state_);
 }
 
 void AutoEnrollmentController::Timeout() {
   // TODO(mnissler): Add UMA to track results of auto-enrollment checks.
   if (client_start_weak_factory_.HasWeakPtrs() &&
       fre_requirement_ != EXPLICITLY_REQUIRED) {
     // If the callbacks to check ownership status or state keys are still
     // 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);
   } 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);
   }
 
   // Reset state.
   Cancel();
 }
 
 }  // namespace chromeos