Refactor verity reads/writes to a separate fucntion
Test: th & serve an OTA with veity enabled, vabc disabled
Change-Id: Ib1d5549ac615504a47c96a12b046975cfff01886
diff --git a/payload_consumer/filesystem_verifier_action.cc b/payload_consumer/filesystem_verifier_action.cc
index 9221caa..aa2fbaa 100644
--- a/payload_consumer/filesystem_verifier_action.cc
+++ b/payload_consumer/filesystem_verifier_action.cc
@@ -40,6 +40,37 @@
using brillo::data_encoding::Base64Encode;
using std::string;
+// On a partition with verity enabled, we expect to see the following format:
+// ===================================================
+// Normal Filesystem Data
+// (this should take most of the space, like over 90%)
+// ===================================================
+// Hash tree
+// ~0.8% (e.g. 16M for 2GB image)
+// ===================================================
+// FEC data
+// ~0.8%
+// ===================================================
+// Footer
+// 4K
+// ===================================================
+
+// For OTA that doesn't do on device verity computation, hash tree and fec data
+// are written during DownloadAction as a regular InstallOp, so no special
+// handling needed, we can just read the entire partition in 1 go.
+
+// Verity enabled case: Only Normal FS data is written during download action.
+// When hasing the entire partition, we will need to build the hash tree, write
+// it to disk, then build FEC, and write it to disk. Therefore, it is important
+// that we finish writing hash tree before we attempt to read & hash it. The
+// same principal applies to FEC data.
+
+// |verity_writer_| handles building and
+// writing of FEC/HashTree, we just need to be careful when reading.
+// Specifically, we must stop at beginning of Hash tree, let |verity_writer_|
+// write both hash tree and FEC, then continue reading the remaining part of
+// partition.
+
namespace chromeos_update_engine {
namespace {
@@ -197,15 +228,23 @@
hasher_ = std::make_unique<HashCalculator>();
offset_ = 0;
+ filesystem_data_end_ = partition_size_;
+ CHECK_LE(partition.hash_tree_offset, partition.fec_offset)
+ << " Hash tree is expected to come before FEC data";
+ if (partition.hash_tree_offset != 0) {
+ filesystem_data_end_ = partition.hash_tree_offset;
+ } else if (partition.fec_offset != 0) {
+ filesystem_data_end_ = partition.fec_offset;
+ }
if (ShouldWriteVerity()) {
- if (!verity_writer_->Init(partition, read_fd_, write_fd_)) {
+ if (!verity_writer_->Init(partition)) {
Cleanup(ErrorCode::kVerityCalculationError);
return;
}
}
// Start the first read.
- ScheduleRead();
+ ScheduleFileSystemRead();
}
bool FilesystemVerifierAction::ShouldWriteVerity() {
@@ -216,24 +255,45 @@
(partition.hash_tree_size > 0 || partition.fec_size > 0);
}
-void FilesystemVerifierAction::ScheduleRead() {
- const InstallPlan::Partition& partition =
- install_plan_.partitions[partition_index_];
+void FilesystemVerifierAction::ReadVerityAndFooter() {
+ if (ShouldWriteVerity()) {
+ if (!verity_writer_->Finalize(read_fd_, write_fd_)) {
+ LOG(ERROR) << "Failed to write hashtree/FEC data.";
+ Cleanup(ErrorCode::kFilesystemVerifierError);
+ return;
+ }
+ }
+ auto bytes_to_read = partition_size_ - filesystem_data_end_;
+ // Since we handed our |read_fd_| to verity_writer_ during |Finalize()| call,
+ // fd's position could have been changed. Re-seek.
+ read_fd_->Seek(filesystem_data_end_, SEEK_SET);
+ while (bytes_to_read > 0) {
+ const auto read_size = std::min<size_t>(buffer_.size(), bytes_to_read);
+ auto bytes_read = read_fd_->Read(buffer_.data(), read_size);
+ if (bytes_read <= 0) {
+ PLOG(ERROR) << "Failed to read hash tree " << bytes_read;
+ Cleanup(ErrorCode::kFilesystemVerifierError);
+ return;
+ }
+ if (!hasher_->Update(buffer_.data(), bytes_read)) {
+ LOG(ERROR) << "Unable to update the hash.";
+ Cleanup(ErrorCode::kError);
+ return;
+ }
+ bytes_to_read -= bytes_read;
+ }
+ FinishPartitionHashing();
+}
+void FilesystemVerifierAction::ScheduleFileSystemRead() {
// We can only start reading anything past |hash_tree_offset| after we have
// already read all the data blocks that the hash tree covers. The same
// applies to FEC.
- uint64_t read_end = partition_size_;
- if (partition.hash_tree_size != 0 &&
- offset_ < partition.hash_tree_data_offset + partition.hash_tree_data_size)
- read_end = std::min(read_end, partition.hash_tree_offset);
- if (partition.fec_size != 0 &&
- offset_ < partition.fec_data_offset + partition.fec_data_size)
- read_end = std::min(read_end, partition.fec_offset);
- size_t bytes_to_read =
- std::min(static_cast<uint64_t>(buffer_.size()), read_end - offset_);
+
+ size_t bytes_to_read = std::min(static_cast<uint64_t>(buffer_.size()),
+ filesystem_data_end_ - offset_);
if (!bytes_to_read) {
- FinishPartitionHashing();
+ ReadVerityAndFooter();
return;
}
@@ -279,19 +339,19 @@
install_plan_.partitions.size());
if (ShouldWriteVerity()) {
if (!verity_writer_->Update(offset_, buffer_.data(), bytes_read)) {
+ LOG(ERROR) << "Unable to update verity";
Cleanup(ErrorCode::kVerityCalculationError);
return;
}
}
offset_ += bytes_read;
-
- if (offset_ == partition_size_) {
- FinishPartitionHashing();
+ if (offset_ == filesystem_data_end_) {
+ ReadVerityAndFooter();
return;
}
- ScheduleRead();
+ ScheduleFileSystemRead();
}
void FilesystemVerifierAction::FinishPartitionHashing() {
diff --git a/payload_consumer/filesystem_verifier_action.h b/payload_consumer/filesystem_verifier_action.h
index b6df4b8..1f527c9 100644
--- a/payload_consumer/filesystem_verifier_action.h
+++ b/payload_consumer/filesystem_verifier_action.h
@@ -91,8 +91,12 @@
// remaining to be hashed, it finishes the action.
void StartPartitionHashing();
- // Schedules the asynchronous read of the filesystem.
- void ScheduleRead();
+ // Schedules the asynchronous read of the filesystem part of this
+ // partition(not including hashtree/verity).
+ void ScheduleFileSystemRead();
+
+ // Read the verity part of this partition.(hash tree and FEC)
+ void ReadVerityAndFooter();
// Called from the main loop when a single read from |src_stream_| succeeds or
// fails, calling OnReadDoneCallback() and OnReadErrorCallback() respectively.
@@ -155,6 +159,9 @@
// The byte offset that we are reading in the current partition.
uint64_t offset_{0};
+ // The end offset of filesystem data, first byte position of hashtree.
+ uint64_t filesystem_data_end_{0};
+
// An observer that observes progress updates of this action.
FilesystemVerifyDelegate* delegate_{};
diff --git a/payload_consumer/verity_writer_android.cc b/payload_consumer/verity_writer_android.cc
index 864d9a1..01d8977 100644
--- a/payload_consumer/verity_writer_android.cc
+++ b/payload_consumer/verity_writer_android.cc
@@ -29,6 +29,7 @@
}
#include "update_engine/common/utils.h"
+#include "update_engine/payload_consumer/cached_file_descriptor.h"
#include "update_engine/payload_consumer/file_descriptor.h"
namespace chromeos_update_engine {
@@ -40,16 +41,7 @@
} // namespace verity_writer
bool VerityWriterAndroid::Init(const InstallPlan::Partition& partition) {
- auto read_fd = FileDescriptorPtr(new EintrSafeFileDescriptor());
- TEST_AND_RETURN_FALSE(read_fd->Open(partition.target_path.c_str(), O_RDWR));
- return Init(partition, read_fd, read_fd);
-}
-bool VerityWriterAndroid::Init(const InstallPlan::Partition& partition,
- FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) {
partition_ = &partition;
- read_fd_ = read_fd;
- write_fd_ = write_fd;
if (partition_->hash_tree_size != 0 || partition_->fec_size != 0) {
utils::SetBlockDeviceReadOnly(partition_->target_path, false);
@@ -82,47 +74,57 @@
const uint8_t* buffer,
size_t size) {
if (partition_->hash_tree_size != 0) {
- uint64_t hash_tree_data_end =
+ const uint64_t hash_tree_data_end =
partition_->hash_tree_data_offset + partition_->hash_tree_data_size;
- uint64_t start_offset = std::max(offset, partition_->hash_tree_data_offset);
- uint64_t end_offset = std::min(offset + size, hash_tree_data_end);
+ const uint64_t start_offset =
+ std::max(offset, partition_->hash_tree_data_offset);
+ if (offset + size > hash_tree_data_end) {
+ LOG(WARNING)
+ << "Reading past hash_tree_data_end, something is probably "
+ "wrong, might cause incorrect hash of partitions. offset: "
+ << offset << " size: " << size
+ << " hash_tree_data_end: " << hash_tree_data_end;
+ }
+ const uint64_t end_offset = std::min(offset + size, hash_tree_data_end);
if (start_offset < end_offset) {
TEST_AND_RETURN_FALSE(hash_tree_builder_->Update(
buffer + start_offset - offset, end_offset - start_offset));
if (end_offset == hash_tree_data_end) {
- // All hash tree data blocks has been hashed, write hash tree to disk.
- LOG(INFO) << "Writing verity hash tree to " << partition_->target_path;
- TEST_AND_RETURN_FALSE(hash_tree_builder_->BuildHashTree());
- TEST_AND_RETURN_FALSE_ERRNO(
- write_fd_->Seek(partition_->hash_tree_offset, SEEK_SET));
- auto success = hash_tree_builder_->WriteHashTree(
- [write_fd_(this->write_fd_)](auto data, auto size) {
- return utils::WriteAll(write_fd_, data, size);
- });
- // hashtree builder already prints error messages.
- if (!success) {
- return false;
- }
- hash_tree_builder_.reset();
+ LOG(INFO)
+ << "Read everything before hash tree. Ready to write hash tree.";
}
}
}
+
+ return true;
+}
+
+bool VerityWriterAndroid::Finalize(FileDescriptorPtr read_fd,
+ FileDescriptorPtr write_fd) {
+ // All hash tree data blocks has been hashed, write hash tree to disk.
+ LOG(INFO) << "Writing verity hash tree to " << partition_->target_path;
+ TEST_AND_RETURN_FALSE(hash_tree_builder_->BuildHashTree());
+ TEST_AND_RETURN_FALSE_ERRNO(
+ write_fd->Seek(partition_->hash_tree_offset, SEEK_SET));
+ auto success =
+ hash_tree_builder_->WriteHashTree([write_fd](auto data, auto size) {
+ return utils::WriteAll(write_fd, data, size);
+ });
+ // hashtree builder already prints error messages.
+ TEST_AND_RETURN_FALSE(success);
+ hash_tree_builder_.reset();
if (partition_->fec_size != 0) {
- uint64_t fec_data_end =
- partition_->fec_data_offset + partition_->fec_data_size;
- if (offset < fec_data_end && offset + size >= fec_data_end) {
- LOG(INFO) << "Writing verity FEC to " << partition_->target_path;
- TEST_AND_RETURN_FALSE(EncodeFEC(read_fd_,
- write_fd_,
- partition_->fec_data_offset,
- partition_->fec_data_size,
- partition_->fec_offset,
- partition_->fec_size,
- partition_->fec_roots,
- partition_->block_size,
- false /* verify_mode */));
- }
+ LOG(INFO) << "Writing verity FEC to " << partition_->target_path;
+ TEST_AND_RETURN_FALSE(EncodeFEC(read_fd,
+ write_fd,
+ partition_->fec_data_offset,
+ partition_->fec_data_size,
+ partition_->fec_offset,
+ partition_->fec_size,
+ partition_->fec_roots,
+ partition_->block_size,
+ false /* verify_mode */));
}
return true;
}
@@ -147,6 +149,10 @@
init_rs_char(FEC_PARAMS(fec_roots)), &free_rs_char);
TEST_AND_RETURN_FALSE(rs_char != nullptr);
+ // Cache at most 1MB of fec data, in VABC, we need to re-open fd if we
+ // perform a read() operation after write(). So reduce the number of writes
+ // can save unnecessary re-opens.
+ write_fd = std::make_shared<CachedFileDescriptor>(write_fd, 1 * (1 << 20));
for (size_t i = 0; i < rounds; i++) {
// Encodes |block_size| number of rs blocks each round so that we can read
// one block each time instead of 1 byte to increase random read
@@ -190,14 +196,15 @@
TEST_AND_RETURN_FALSE(fec == fec_read);
} else {
CHECK(write_fd);
- if (!utils::PWriteAll(write_fd, fec.data(), fec.size(), fec_offset)) {
+ write_fd->Seek(fec_offset, SEEK_SET);
+ if (!utils::WriteAll(write_fd, fec.data(), fec.size())) {
PLOG(ERROR) << "EncodeFEC write() failed";
return false;
}
}
fec_offset += fec.size();
}
-
+ write_fd->Flush();
return true;
}
diff --git a/payload_consumer/verity_writer_android.h b/payload_consumer/verity_writer_android.h
index 7dfac0f..e95f188 100644
--- a/payload_consumer/verity_writer_android.h
+++ b/payload_consumer/verity_writer_android.h
@@ -32,11 +32,9 @@
VerityWriterAndroid() = default;
~VerityWriterAndroid() override = default;
- bool Init(const InstallPlan::Partition& partition,
- FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) override;
bool Init(const InstallPlan::Partition& partition);
bool Update(uint64_t offset, const uint8_t* buffer, size_t size) override;
+ bool Finalize(FileDescriptorPtr read_fd, FileDescriptorPtr write_fd) override;
// Read [data_offset : data_offset + data_size) from |path| and encode FEC
// data, if |verify_mode|, then compare the encoded FEC with the one in
@@ -65,10 +63,7 @@
private:
const InstallPlan::Partition* partition_ = nullptr;
- FileDescriptorPtr read_fd_;
- FileDescriptorPtr write_fd_;
std::unique_ptr<HashTreeBuilder> hash_tree_builder_;
-
DISALLOW_COPY_AND_ASSIGN(VerityWriterAndroid);
};
diff --git a/payload_consumer/verity_writer_android_unittest.cc b/payload_consumer/verity_writer_android_unittest.cc
index ec22ffb..81d8ca2 100644
--- a/payload_consumer/verity_writer_android_unittest.cc
+++ b/payload_consumer/verity_writer_android_unittest.cc
@@ -16,11 +16,14 @@
#include "update_engine/payload_consumer/verity_writer_android.h"
+#include <fcntl.h>
+
#include <brillo/secure_blob.h>
#include <gtest/gtest.h>
#include "update_engine/common/test_utils.h"
#include "update_engine/common/utils.h"
+#include "update_engine/payload_consumer/file_descriptor.h"
namespace chromeos_update_engine {
@@ -35,10 +38,13 @@
partition_.hash_tree_size = 4096;
partition_.hash_tree_algorithm = "sha1";
partition_.fec_roots = 2;
+ partition_fd_ = std::make_shared<EintrSafeFileDescriptor>();
+ partition_fd_->Open(partition_.target_path.c_str(), O_RDWR);
}
VerityWriterAndroid verity_writer_;
InstallPlan::Partition partition_;
+ FileDescriptorPtr partition_fd_;
ScopedTempFile temp_file_;
};
@@ -46,8 +52,9 @@
brillo::Blob part_data(8192);
test_utils::WriteFileVector(partition_.target_path, part_data);
ASSERT_TRUE(verity_writer_.Init(partition_));
- EXPECT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
- EXPECT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
+ ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
+ ASSERT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
+ ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// dd if=/dev/zero bs=4096 count=1 2>/dev/null | sha1sum | xxd -r -p |
@@ -56,7 +63,7 @@
0x1d, 0xf3, 0xbf, 0xb2, 0x6b, 0x4f, 0xb7,
0xcd, 0x95, 0xfb, 0x7b, 0xff, 0x1d};
memcpy(part_data.data() + 4096, hash.data(), hash.size());
- EXPECT_EQ(part_data, actual_part);
+ ASSERT_EQ(part_data, actual_part);
}
TEST_F(VerityWriterAndroidTest, NoOpTest) {
@@ -64,19 +71,19 @@
partition_.hash_tree_size = 0;
brillo::Blob part_data(4096);
ASSERT_TRUE(verity_writer_.Init(partition_));
- EXPECT_TRUE(verity_writer_.Update(0, part_data.data(), part_data.size()));
- EXPECT_TRUE(verity_writer_.Update(4096, part_data.data(), part_data.size()));
- EXPECT_TRUE(verity_writer_.Update(8192, part_data.data(), part_data.size()));
+ ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), part_data.size()));
+ ASSERT_TRUE(verity_writer_.Update(4096, part_data.data(), part_data.size()));
+ ASSERT_TRUE(verity_writer_.Update(8192, part_data.data(), part_data.size()));
}
TEST_F(VerityWriterAndroidTest, InvalidHashAlgorithmTest) {
partition_.hash_tree_algorithm = "sha123";
- EXPECT_FALSE(verity_writer_.Init(partition_));
+ ASSERT_FALSE(verity_writer_.Init(partition_));
}
TEST_F(VerityWriterAndroidTest, WrongHashTreeSizeTest) {
partition_.hash_tree_size = 8192;
- EXPECT_FALSE(verity_writer_.Init(partition_));
+ ASSERT_FALSE(verity_writer_.Init(partition_));
}
TEST_F(VerityWriterAndroidTest, SHA256Test) {
@@ -84,8 +91,9 @@
brillo::Blob part_data(8192);
test_utils::WriteFileVector(partition_.target_path, part_data);
ASSERT_TRUE(verity_writer_.Init(partition_));
- EXPECT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
- EXPECT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
+ ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), 4096));
+ ASSERT_TRUE(verity_writer_.Update(4096, part_data.data() + 4096, 4096));
+ ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// dd if=/dev/zero bs=4096 count=1 2>/dev/null | sha256sum | xxd -r -p |
@@ -95,7 +103,7 @@
0x4f, 0x58, 0x05, 0xff, 0x7c, 0xb4, 0x7c, 0x7a,
0x85, 0xda, 0xbd, 0x8b, 0x48, 0x89, 0x2c, 0xa7};
memcpy(part_data.data() + 4096, hash.data(), hash.size());
- EXPECT_EQ(part_data, actual_part);
+ ASSERT_EQ(part_data, actual_part);
}
TEST_F(VerityWriterAndroidTest, FECTest) {
@@ -106,7 +114,8 @@
brillo::Blob part_data(3 * 4096, 0x1);
test_utils::WriteFileVector(partition_.target_path, part_data);
ASSERT_TRUE(verity_writer_.Init(partition_));
- EXPECT_TRUE(verity_writer_.Update(0, part_data.data(), part_data.size()));
+ ASSERT_TRUE(verity_writer_.Update(0, part_data.data(), part_data.size()));
+ ASSERT_TRUE(verity_writer_.Finalize(partition_fd_, partition_fd_));
brillo::Blob actual_part;
utils::ReadFile(partition_.target_path, &actual_part);
// Write FEC data.
@@ -114,7 +123,7 @@
part_data[i] = 0x8e;
part_data[i + 1] = 0x8f;
}
- EXPECT_EQ(part_data, actual_part);
+ ASSERT_EQ(part_data, actual_part);
}
} // namespace chromeos_update_engine
diff --git a/payload_consumer/verity_writer_interface.h b/payload_consumer/verity_writer_interface.h
index db7988e..37ed605 100644
--- a/payload_consumer/verity_writer_interface.h
+++ b/payload_consumer/verity_writer_interface.h
@@ -31,9 +31,6 @@
public:
virtual ~VerityWriterInterface() = default;
- virtual bool Init(const InstallPlan::Partition& partition,
- FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) = 0;
virtual bool Init(const InstallPlan::Partition& partition) = 0;
// Update partition data at [offset : offset + size) stored in |buffer|.
// Data not in |hash_tree_data_extent| or |fec_data_extent| is ignored.
@@ -41,6 +38,10 @@
// blocks has passed.
virtual bool Update(uint64_t offset, const uint8_t* buffer, size_t size) = 0;
+ // Write hash tree && FEC data to underlying fd, if they are present
+ virtual bool Finalize(FileDescriptorPtr read_fd,
+ FileDescriptorPtr write_fd) = 0;
+
protected:
VerityWriterInterface() = default;
diff --git a/payload_consumer/verity_writer_stub.cc b/payload_consumer/verity_writer_stub.cc
index 314ec7e..8bff076 100644
--- a/payload_consumer/verity_writer_stub.cc
+++ b/payload_consumer/verity_writer_stub.cc
@@ -26,9 +26,7 @@
}
} // namespace verity_writer
-bool VerityWriterStub::Init(const InstallPlan::Partition& partition,
- FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) {
+bool VerityWriterStub::Init(const InstallPlan::Partition& partition) {
return partition.hash_tree_size == 0 && partition.fec_size == 0;
}
@@ -38,4 +36,9 @@
return true;
}
+bool VerityWriterStub::Finalize(FileDescriptorPtr read_fd,
+ FileDescriptorPtr write_fd) {
+ return true;
+}
+
} // namespace chromeos_update_engine
diff --git a/payload_consumer/verity_writer_stub.h b/payload_consumer/verity_writer_stub.h
index f8d68ca..a20db03 100644
--- a/payload_consumer/verity_writer_stub.h
+++ b/payload_consumer/verity_writer_stub.h
@@ -26,10 +26,9 @@
VerityWriterStub() = default;
~VerityWriterStub() override = default;
- bool Init(const InstallPlan::Partition& partition,
- FileDescriptorPtr read_fd,
- FileDescriptorPtr write_fd) override;
+ bool Init(const InstallPlan::Partition& partition) override;
bool Update(uint64_t offset, const uint8_t* buffer, size_t size) override;
+ bool Finalize(FileDescriptorPtr read_fd, FileDescriptorPtr write_fd) override;
private:
DISALLOW_COPY_AND_ASSIGN(VerityWriterStub);