From 9105f4baeb254e45117ab91c396f0d45a4c8b9ca Mon Sep 17 00:00:00 2001 From: Kelvin Zhang Date: Mon, 26 Apr 2021 13:44:49 -0400 Subject: 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 --- .../filesystem_verifier_action_unittest.cc | 72 ++++++++++++++-------- 1 file changed, 48 insertions(+), 24 deletions(-) (limited to 'payload_consumer/filesystem_verifier_action_unittest.cc') diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc index c1006842..d2a015d8 100644 --- a/payload_consumer/filesystem_verifier_action_unittest.cc +++ b/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 dynamic_control; - auto fake_fd = std::make_shared(); 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>({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) { -- cgit v1.2.3 From 46d6c4987f143e9afbc965bf740873bc1022875f Mon Sep 17 00:00:00 2001 From: Kelvin Zhang Date: Mon, 26 Apr 2021 17:51:25 -0400 Subject: Create a minimal testcase to reproduce silent verity corruption b/186196758 is triggered by the following sequence of events: 1. update_engine finish writing all install ops, emits kEndOfInstall label 2. update_engine opens cow in append mode, invokes InitialiazeAppend(kEndOfInstall) 3. update_engine writes verity data, invokes SnapshotWriter::Finalize() 4. update_engine repeats step 2, but does not write any data after opening SnapshotWriter. Instead, it reads verity and make sure the hash matches what's specified in OTA payload. 5. Reboot device, verity data corrupted, device rollback to slot _a. This is because, during step 4, 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, and determines that everything is fine. However, when calling SnapshotWriter::Finalize(), data after resume label are discarded, therefore verity data is gone. Test: th Bug: 186196758 Change-Id: I0166271b64eb7b574434d617ce730f345ca93ff1 --- payload_consumer/filesystem_verifier_action_unittest.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'payload_consumer/filesystem_verifier_action_unittest.cc') diff --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc index d2a015d8..586662d9 100644 --- a/payload_consumer/filesystem_verifier_action_unittest.cc +++ b/payload_consumer/filesystem_verifier_action_unittest.cc @@ -488,7 +488,6 @@ TEST_F(FilesystemVerifierActionTest, RunWithVABCNoVerity) { } TEST_F(FilesystemVerifierActionTest, ReadAfterWrite) { - constexpr auto BLOCK_SIZE = 4096; ScopedTempFile cow_device_file("cow_device.XXXXXX", true); android::snapshot::CompressedSnapshotWriter snapshot_writer{ {.block_size = BLOCK_SIZE}}; @@ -507,6 +506,12 @@ TEST_F(FilesystemVerifierActionTest, ReadAfterWrite) { ASSERT_TRUE(snapshot_writer.Finalize()); cow_reader = snapshot_writer.OpenReader(); ASSERT_NE(cow_reader, nullptr); + std::vector read_back; + read_back.resize(buffer.size()); + cow_reader->Seek(BLOCK_SIZE, SEEK_SET); + const auto bytes_read = cow_reader->Read(read_back.data(), read_back.size()); + ASSERT_EQ((size_t)(bytes_read), BLOCK_SIZE); + ASSERT_EQ(read_back, buffer); } } // namespace chromeos_update_engine -- cgit v1.2.3