Fix verity discarded bug

If update_engine opens CowWriterFileDescriptor w/o writing anything,
data past the resume label is readable while fd is open, but will
be discarded once the fd is closed. Such "phantom read" causes
inconsistency. This CL contains two changes to address the above bug:

1. When device reboots after update, all I/O are served by snapuserd.
update_engine should use snapuserd for verification to emulate bahvior
of device after reboot.

2. When a CowWriterFd is opened, don't call Finalize() if no verity is
written. Since past-the-end data is discarded when we call Finalize()

Test: th

Bug: 186196758

Change-Id: Ia1d31b671c16fded7319677fe0397f1288457201

Author: zhangxp1998

Diff for: common/hash_calculator.cc

@@ -95,6 +95,11 @@ bool HashCalculator::RawHashOfData(const brillo::Blob& data,
   return RawHashOfBytes(data.data(), data.size(), out_hash);
 }
 
+bool HashCalculator::RawHashOfFile(const string& name, brillo::Blob* out_hash) {
+  const auto file_size = utils::FileSize(name);
+  return RawHashOfFile(name, file_size, out_hash) == file_size;
+}
+
 off_t HashCalculator::RawHashOfFile(const string& name,
                                     off_t length,
                                     brillo::Blob* out_hash) {

Diff for: common/hash_calculator.h

@@ -75,6 +75,7 @@ class HashCalculator {
   static off_t RawHashOfFile(const std::string& name,
                              off_t length,
                              brillo::Blob* out_hash);
+  static bool RawHashOfFile(const std::string& name, brillo::Blob* out_hash);
 
  private:
   // If non-empty, the final raw hash. Will only be set to non-empty when

Diff for: common/utils.h

@@ -399,13 +399,19 @@ class ScopedTempFile {
 
   // If |open_fd| is true, a writable file descriptor will be opened for this
   // file.
-  explicit ScopedTempFile(const std::string& pattern, bool open_fd = false) {
+  // If |truncate_size| is non-zero, truncate file to that size on creation.
+  explicit ScopedTempFile(const std::string& pattern,
+                          bool open_fd = false,
+                          size_t truncate_size = 0) {
     CHECK(utils::MakeTempFile(pattern, &path_, open_fd ? &fd_ : nullptr));
     unlinker_.reset(new ScopedPathUnlinker(path_));
     if (open_fd) {
       CHECK_GE(fd_, 0);
       fd_closer_.reset(new ScopedFdCloser(&fd_));
     }
+    if (truncate_size > 0) {
+      CHECK_EQ(0, truncate(path_.c_str(), truncate_size));
+    }
   }
   virtual ~ScopedTempFile() = default;
 

Diff for: payload_consumer/cow_writer_file_descriptor.cc

@@ -28,7 +28,10 @@ namespace chromeos_update_engine {
 CowWriterFileDescriptor::CowWriterFileDescriptor(
     std::unique_ptr<android::snapshot::ISnapshotWriter> cow_writer)
     : cow_writer_(std::move(cow_writer)),
-      cow_reader_(cow_writer_->OpenReader()) {}
+      cow_reader_(cow_writer_->OpenReader()) {
+  CHECK_NE(cow_writer_, nullptr);
+  CHECK_NE(cow_reader_, nullptr);
+}
 
 bool CowWriterFileDescriptor::Open(const char* path, int flags, mode_t mode) {
   LOG(ERROR) << "CowWriterFileDescriptor doesn't support Open()";
@@ -113,7 +116,17 @@ bool CowWriterFileDescriptor::Flush() {
 
 bool CowWriterFileDescriptor::Close() {
   if (cow_writer_) {
-    TEST_AND_RETURN_FALSE(cow_writer_->Finalize());
+    // b/186196758
+    // When calling
+    // InitializeAppend(kEndOfInstall), the SnapshotWriter only reads up to the
+    // given label. But OpenReader() completely disregards the resume label and
+    // reads all ops. Therefore, update_engine sees the verity data. However,
+    // when calling SnapshotWriter::Finalize(), data after resume label are
+    // discarded, therefore verity data is gone. To prevent phantom reads, don't
+    // call Finalize() unless we actually write something.
+    if (dirty_) {
+      TEST_AND_RETURN_FALSE(cow_writer_->Finalize());
+    }
     cow_writer_ = nullptr;
   }
   if (cow_reader_) {

Diff for: payload_consumer/filesystem_verifier_action.cc

@@ -112,6 +112,14 @@ void FilesystemVerifierAction::Cleanup(ErrorCode code) {
   // This memory is not used anymore.
   buffer_.clear();
 
+  // If we didn't write verity, partitions were maped. Releaase resource now.
+  if (!install_plan_.write_verity &&
+      dynamic_control_->UpdateUsesSnapshotCompression()) {
+    LOG(INFO) << "Not writing verity and VABC is enabled, unmapping all "
+                 "partitions";
+    dynamic_control_->UnmapAllPartitions();
+  }
+
   if (cancelled_)
     return;
   if (code == ErrorCode::kSuccess && HasOutputPipe())
@@ -130,6 +138,21 @@ bool FilesystemVerifierAction::InitializeFdVABC() {
   const InstallPlan::Partition& partition =
       install_plan_.partitions[partition_index_];
 
+  if (!ShouldWriteVerity()) {
+    // In VABC, if we are not writing verity, just map all partitions,
+    // and read using regular fd on |postinstall_mount_device| .
+    // All read will go through snapuserd, which provides a consistent
+    // view: device will use snapuserd to read partition during boot.
+    // b/186196758
+    // Call UnmapAllPartitions() first, because if we wrote verity before, these
+    // writes won't be visible to previously opened snapuserd daemon. To ensure
+    // that we will see the most up to date data from partitions, call Unmap()
+    // then Map() to re-spin daemon.
+    dynamic_control_->UnmapAllPartitions();
+    dynamic_control_->MapAllPartitions();
+    return InitializeFd(partition.readonly_target_path);
+  }
+
   // FilesystemVerifierAction need the read_fd_.
   partition_fd_ =
       dynamic_control_->OpenCowFd(partition.name, partition.source_path, true);

Diff for: payload_consumer/filesystem_verifier_action_unittest.cc

@@ -48,8 +48,23 @@ using testing::SetArgPointee;
 namespace chromeos_update_engine {
 
 class FilesystemVerifierActionTest : public ::testing::Test {
+ public:
+  static constexpr size_t BLOCK_SIZE = 4096;
+  static constexpr size_t PARTITION_SIZE = BLOCK_SIZE * 1024;
+
  protected:
-  void SetUp() override { loop_.SetAsCurrent(); }
+  void SetUp() override {
+    brillo::Blob part_data(PARTITION_SIZE);
+    test_utils::FillWithData(&part_data);
+    ASSERT_TRUE(utils::WriteFile(
+        source_part.path().c_str(), part_data.data(), part_data.size()));
+    // FillWithData() will will with different data next call. We want
+    // source/target partitions to contain different data for testing.
+    test_utils::FillWithData(&part_data);
+    ASSERT_TRUE(utils::WriteFile(
+        target_part.path().c_str(), part_data.data(), part_data.size()));
+    loop_.SetAsCurrent();
+  }
 
   void TearDown() override {
     EXPECT_EQ(0, brillo::MessageLoopRunMaxIterations(&loop_, 1));
@@ -62,11 +77,34 @@ class FilesystemVerifierActionTest : public ::testing::Test {
   void BuildActions(const InstallPlan& install_plan,
                     DynamicPartitionControlInterface* dynamic_control);
 
+  InstallPlan::Partition* AddFakePartition(InstallPlan* install_plan,
+                                           std::string name = "fake_part") {
+    InstallPlan::Partition& part = install_plan->partitions.emplace_back();
+    part.name = name;
+    part.target_path = target_part.path();
+    part.readonly_target_path = part.target_path;
+    part.target_size = PARTITION_SIZE;
+    part.block_size = BLOCK_SIZE;
+    part.source_path = source_part.path();
+    EXPECT_TRUE(
+        HashCalculator::RawHashOfFile(source_part.path(), &part.source_hash));
+    EXPECT_TRUE(
+        HashCalculator::RawHashOfFile(target_part.path(), &part.target_hash));
+    return ∂
+  }
+
   brillo::FakeMessageLoop loop_{nullptr};
   ActionProcessor processor_;
   DynamicPartitionControlStub dynamic_control_stub_;
+  static ScopedTempFile source_part;
+  static ScopedTempFile target_part;
 };
 
+ScopedTempFile FilesystemVerifierActionTest::source_part{
+    "source_part.XXXXXX", false, PARTITION_SIZE};
+ScopedTempFile FilesystemVerifierActionTest::target_part{
+    "target_part.XXXXXX", false, PARTITION_SIZE};
+
 class FilesystemVerifierActionTestDelegate : public ActionProcessorDelegate {
  public:
   FilesystemVerifierActionTestDelegate()
@@ -406,32 +444,27 @@ TEST_F(FilesystemVerifierActionTest, RunAsRootSkipWriteVerityTest) {
 
 TEST_F(FilesystemVerifierActionTest, RunWithVABCNoVerity) {
   InstallPlan install_plan;
-  InstallPlan::Partition& part = install_plan.partitions.emplace_back();
-  part.name = "fake_part";
-  part.target_path = "/dev/fake_target_path";
-  part.target_size = 4096 * 4096;
-  part.block_size = 4096;
-  part.source_path = "/dev/fake_source_path";
-  part.fec_size = 0;
-  part.hash_tree_size = 0;
-  part.target_hash.clear();
-  part.source_hash.clear();
+  auto part_ptr = AddFakePartition(&install_plan);
+  ASSERT_NE(part_ptr, nullptr);
+  InstallPlan::Partition& part = *part_ptr;
+  part.target_path = "Shouldn't attempt to open this path";
 
   NiceMock<MockDynamicPartitionControl> dynamic_control;
-  auto fake_fd = std::make_shared<FakeFileDescriptor>();
 
   ON_CALL(dynamic_control, GetDynamicPartitionsFeatureFlag())
       .WillByDefault(Return(FeatureFlag(FeatureFlag::Value::LAUNCH)));
   ON_CALL(dynamic_control, UpdateUsesSnapshotCompression())
       .WillByDefault(Return(true));
-  ON_CALL(dynamic_control, OpenCowFd(_, _, _)).WillByDefault(Return(fake_fd));
   ON_CALL(dynamic_control, IsDynamicPartition(part.name, _))
       .WillByDefault(Return(true));
 
   EXPECT_CALL(dynamic_control, UpdateUsesSnapshotCompression())
       .Times(AtLeast(1));
+  // Since we are not writing verity, we should not attempt to OpenCowFd()
+  // reads should go through regular file descriptors on mapped partitions.
   EXPECT_CALL(dynamic_control, OpenCowFd(part.name, {part.source_path}, _))
-      .Times(1);
+      .Times(0);
+  EXPECT_CALL(dynamic_control, MapAllPartitions()).Times(AtLeast(1));
   EXPECT_CALL(dynamic_control, ListDynamicPartitionsForSlot(_, _, _))
       .WillRepeatedly(
           DoAll(SetArgPointee<2, std::vector<std::string>>({part.name}),
@@ -451,16 +484,7 @@ TEST_F(FilesystemVerifierActionTest, RunWithVABCNoVerity) {
 
   ASSERT_FALSE(processor_.IsRunning());
   ASSERT_TRUE(delegate.ran());
-  // Filesystem verifier will fail, because we set an empty hash
-  ASSERT_EQ(ErrorCode::kNewRootfsVerificationError, delegate.code());
-  const auto& read_pos = fake_fd->GetReadOps();
-  size_t expected_offset = 0;
-  for (const auto& [off, size] : read_pos) {
-    ASSERT_EQ(off, expected_offset);
-    expected_offset += size;
-  }
-  const auto actual_read_size = expected_offset;
-  ASSERT_EQ(actual_read_size, part.target_size);
+  ASSERT_EQ(ErrorCode::kSuccess, delegate.code());
 }
 
 TEST_F(FilesystemVerifierActionTest, ReadAfterWrite) {