From aab166132a59ab19d59bbe6f78b2d42bad162697 Mon Sep 17 00:00:00 2001 From: Alessio Balsini Date: Thu, 11 Feb 2021 19:07:41 +0000 Subject: [PATCH 1/2] libsnapshot: COW size is computed within the container capability Enforce the checking of what chunk/sector/byte is written to the COW size calculator to avoid possible overflows in the container. If an undesired behavior occurs, the COW computation is aborted and, the class user will be notified with an empty returned value. Bug: 176972301 Test: vts_libsnapshot_test Change-Id: I29f7909780853434a09032c27c943f505c6d0d19 --- fs_mgr/libsnapshot/dm_snapshot_internals.h | 51 +++++++++++++++++-- fs_mgr/libsnapshot/partition_cow_creator.cpp | 8 +-- fs_mgr/libsnapshot/partition_cow_creator.h | 2 +- .../partition_cow_creator_test.cpp | 4 ++ 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/fs_mgr/libsnapshot/dm_snapshot_internals.h b/fs_mgr/libsnapshot/dm_snapshot_internals.h index fef256d03..444d5e11b 100644 --- a/fs_mgr/libsnapshot/dm_snapshot_internals.h +++ b/fs_mgr/libsnapshot/dm_snapshot_internals.h @@ -14,8 +14,10 @@ #pragma once +#include #include +#include #include namespace android { @@ -31,14 +33,41 @@ class DmSnapCowSizeCalculator { void WriteByte(uint64_t address) { WriteSector(address / sector_bytes_); } void WriteSector(uint64_t sector) { WriteChunk(sector / chunk_sectors_); } void WriteChunk(uint64_t chunk_id) { - if (modified_chunks_.size() <= chunk_id) { - modified_chunks_.resize(chunk_id + 1, false); + if (!valid_) { + return; } + + if (modified_chunks_.size() <= chunk_id) { + if (modified_chunks_.max_size() <= chunk_id) { + LOG(ERROR) << "Invalid COW size, chunk_id is too large."; + valid_ = false; + return; + } + modified_chunks_.resize(chunk_id + 1, false); + if (modified_chunks_.size() <= chunk_id) { + LOG(ERROR) << "Invalid COW size, chunk_id is too large."; + valid_ = false; + return; + } + } + modified_chunks_[chunk_id] = true; } - uint64_t cow_size_bytes() const { return cow_size_sectors() * sector_bytes_; } - uint64_t cow_size_sectors() const { return cow_size_chunks() * chunk_sectors_; } + std::optional cow_size_bytes() const { + auto sectors = cow_size_sectors(); + if (!sectors) { + return std::nullopt; + } + return sectors.value() * sector_bytes_; + } + std::optional cow_size_sectors() const { + auto chunks = cow_size_chunks(); + if (!chunks) { + return std::nullopt; + } + return chunks.value() * chunk_sectors_; + } /* * The COW device has a precise internal structure as follows: @@ -56,7 +85,12 @@ class DmSnapCowSizeCalculator { * - chunks addressable by previous map (exceptions_per_chunk) * - 1 extra chunk */ - uint64_t cow_size_chunks() const { + std::optional cow_size_chunks() const { + if (!valid_) { + LOG(ERROR) << "Invalid COW size."; + return std::nullopt; + } + uint64_t modified_chunks_count = 0; uint64_t cow_chunks = 0; @@ -102,6 +136,13 @@ class DmSnapCowSizeCalculator { */ const uint64_t exceptions_per_chunk; + /* + * Validity check for the container. + * It may happen that the caller attempts the write of an invalid chunk + * identifier, and this misbehavior is accounted and stored in this value. + */ + bool valid_ = true; + /* * |modified_chunks_| is a container that keeps trace of the modified * chunks. diff --git a/fs_mgr/libsnapshot/partition_cow_creator.cpp b/fs_mgr/libsnapshot/partition_cow_creator.cpp index da6fc9d20..6002043ab 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.cpp +++ b/fs_mgr/libsnapshot/partition_cow_creator.cpp @@ -142,11 +142,11 @@ void WriteExtent(DmSnapCowSizeCalculator* sc, const chromeos_update_engine::Exte } } -uint64_t PartitionCowCreator::GetCowSize() { +std::optional PartitionCowCreator::GetCowSize() { if (compression_enabled) { if (update == nullptr || !update->has_estimate_cow_size()) { LOG(ERROR) << "Update manifest does not include a COW size"; - return 0; + return std::nullopt; } // Add an extra 2MB of wiggle room for any minor differences in labels/metadata @@ -239,7 +239,7 @@ std::optional PartitionCowCreator::Run() { } // Compute the COW partition size. - uint64_t cow_partition_size = std::min(cow_size, free_region_length); + uint64_t cow_partition_size = std::min(cow_size.value(), free_region_length); // Round it down to the nearest logical block. Logical partitions must be a multiple // of logical blocks. cow_partition_size &= ~(logical_block_size - 1); @@ -247,7 +247,7 @@ std::optional PartitionCowCreator::Run() { // Assign cow_partition_usable_regions to indicate what regions should the COW partition uses. ret.cow_partition_usable_regions = std::move(free_regions); - auto cow_file_size = cow_size - cow_partition_size; + auto cow_file_size = cow_size.value() - cow_partition_size; // Round it up to the nearest sector. cow_file_size += kSectorSize - 1; cow_file_size &= ~(kSectorSize - 1); diff --git a/fs_mgr/libsnapshot/partition_cow_creator.h b/fs_mgr/libsnapshot/partition_cow_creator.h index 64d186b53..84372de9c 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator.h +++ b/fs_mgr/libsnapshot/partition_cow_creator.h @@ -68,7 +68,7 @@ struct PartitionCowCreator { private: bool HasExtent(Partition* p, Extent* e); - uint64_t GetCowSize(); + std::optional GetCowSize(); }; } // namespace snapshot diff --git a/fs_mgr/libsnapshot/partition_cow_creator_test.cpp b/fs_mgr/libsnapshot/partition_cow_creator_test.cpp index e4b476f9b..de35c132d 100644 --- a/fs_mgr/libsnapshot/partition_cow_creator_test.cpp +++ b/fs_mgr/libsnapshot/partition_cow_creator_test.cpp @@ -308,6 +308,10 @@ TEST(DmSnapshotInternals, CowSizeCalculator) { cc.WriteByte(b); ASSERT_EQ(cc.cow_size_sectors(), 40); } + + // Write a byte that would surely overflow the counter + cc.WriteChunk(std::numeric_limits::max()); + ASSERT_FALSE(cc.cow_size_sectors().has_value()); } void BlocksToExtents(const std::vector& blocks, From 2f9a4ff57d228facff0d38eaf37480878640a8bd Mon Sep 17 00:00:00 2001 From: Alessio Balsini Date: Tue, 16 Feb 2021 11:00:52 +0000 Subject: [PATCH 2/2] COW file size: better explain the exception size The exception size was hardcoded and its explanation hidden in one of the comments. Move it to a separate constant e better explain why that is 64 * 2 / 8. Bug: 176972301 Test: m Signed-off-by: Alessio Balsini Change-Id: Ifcb527540882222916ada07dacf3f76f87609539 --- fs_mgr/libsnapshot/dm_snapshot_internals.h | 24 +++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/fs_mgr/libsnapshot/dm_snapshot_internals.h b/fs_mgr/libsnapshot/dm_snapshot_internals.h index 444d5e11b..ed77c1526 100644 --- a/fs_mgr/libsnapshot/dm_snapshot_internals.h +++ b/fs_mgr/libsnapshot/dm_snapshot_internals.h @@ -28,7 +28,7 @@ class DmSnapCowSizeCalculator { DmSnapCowSizeCalculator(unsigned int sector_bytes, unsigned int chunk_sectors) : sector_bytes_(sector_bytes), chunk_sectors_(chunk_sectors), - exceptions_per_chunk(chunk_sectors_ * sector_bytes_ / (64 * 2 / 8)) {} + exceptions_per_chunk(chunk_sectors_ * sector_bytes_ / exception_size_bytes) {} void WriteByte(uint64_t address) { WriteSector(address / sector_bytes_); } void WriteSector(uint64_t sector) { WriteChunk(sector / chunk_sectors_); } @@ -124,18 +124,22 @@ class DmSnapCowSizeCalculator { const uint64_t chunk_sectors_; /* - * The COW device stores tables to map the modified chunks. Each table - * has the size of exactly 1 chunk. - * Each row of the table (also called exception in the kernel) contains two - * 64 bit indices to identify the corresponding chunk, and this 128 bit row - * size is a constant. - * The number of exceptions that each table can contain determines the - * number of data chunks that separate two consecutive tables. This value - * is then fundamental to compute the space overhead introduced by the - * tables in COW devices. + * The COW device stores tables to map the modified chunks. Each table has + * the size of exactly 1 chunk. + * Each entry of the table is called exception and the number of exceptions + * that each table can contain determines the number of data chunks that + * separate two consecutive tables. This value is then fundamental to + * compute the space overhead introduced by the tables in COW devices. */ const uint64_t exceptions_per_chunk; + /* + * Each row of the table (called exception in the kernel) contains two + * 64 bit indices to identify the corresponding chunk, and this 128 bit + * pair is constant in size. + */ + static constexpr unsigned int exception_size_bytes = 64 * 2 / 8; + /* * Validity check for the container. * It may happen that the caller attempts the write of an invalid chunk