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
diff --git a/common/hash_calculator.cc b/common/hash_calculator.cc
index d010a53..60812d5 100644
--- a/common/hash_calculator.cc
+++ b/common/hash_calculator.cc
@@ -95,6 +95,11 @@
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 --git a/common/hash_calculator.h b/common/hash_calculator.h
index b7e4d86..4426128 100644
--- a/common/hash_calculator.h
+++ b/common/hash_calculator.h
@@ -75,6 +75,7 @@
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 --git a/common/utils.h b/common/utils.h
index 5f6e475..59f236e 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -399,13 +399,19 @@
// 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 --git a/payload_consumer/cow_writer_file_descriptor.cc b/payload_consumer/cow_writer_file_descriptor.cc
index d8c7afb..2de6664 100644
--- a/payload_consumer/cow_writer_file_descriptor.cc
+++ b/payload_consumer/cow_writer_file_descriptor.cc
@@ -28,7 +28,10 @@
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::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 --git a/payload_consumer/filesystem_verifier_action.cc b/payload_consumer/filesystem_verifier_action.cc
index 09dc638..b14cbc8 100644
--- a/payload_consumer/filesystem_verifier_action.cc
+++ b/payload_consumer/filesystem_verifier_action.cc
@@ -112,6 +112,14 @@
// 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 @@
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 --git a/payload_consumer/filesystem_verifier_action_unittest.cc b/payload_consumer/filesystem_verifier_action_unittest.cc
index c100684..d2a015d 100644
--- a/payload_consumer/filesystem_verifier_action_unittest.cc
+++ b/payload_consumer/filesystem_verifier_action_unittest.cc
@@ -48,8 +48,23 @@
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 @@
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, 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 @@
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) {