From 42c2733d6ca4296a44a06a19207ae142eedd18fb Mon Sep 17 00:00:00 2001 From: David Anderson Date: Sat, 2 May 2020 15:59:30 -0700 Subject: [PATCH] liblp: Add integer overflow checks when aligning. This will prevent ubsan crashes on invalid inputs. Bug: 155510366 Test: liblp_test gtest Change-Id: Id6dd8badd0025d6cac3113c3f9076ea3f4d9c175 --- fs_mgr/liblp/builder.cpp | 62 ++++++++++++++++++++++------ fs_mgr/liblp/builder_test.cpp | 17 +++++++- fs_mgr/liblp/include/liblp/builder.h | 2 +- fs_mgr/liblp/utility.h | 23 ++++++++--- fs_mgr/liblp/utility_test.cpp | 31 ++++++++++---- 5 files changed, 106 insertions(+), 29 deletions(-) diff --git a/fs_mgr/liblp/builder.cpp b/fs_mgr/liblp/builder.cpp index dc3b985a9..c37d70e05 100644 --- a/fs_mgr/liblp/builder.cpp +++ b/fs_mgr/liblp/builder.cpp @@ -19,6 +19,7 @@ #include #include +#include #include @@ -369,7 +370,10 @@ bool MetadataBuilder::Init(const std::vector& block_devices, } // Align the metadata size up to the nearest sector. - metadata_max_size = AlignTo(metadata_max_size, LP_SECTOR_SIZE); + if (!AlignTo(metadata_max_size, LP_SECTOR_SIZE, &metadata_max_size)) { + LERROR << "Max metadata size " << metadata_max_size << " is too large."; + return false; + } // Validate and build the block device list. uint32_t logical_block_size = 0; @@ -401,10 +405,15 @@ bool MetadataBuilder::Init(const std::vector& block_devices, // untouched to be compatible code that looks for an MBR. Thus we // start counting free sectors at sector 1, not 0. uint64_t free_area_start = LP_SECTOR_SIZE; + bool ok; if (out.alignment) { - free_area_start = AlignTo(free_area_start, out.alignment); + ok = AlignTo(free_area_start, out.alignment, &free_area_start); } else { - free_area_start = AlignTo(free_area_start, logical_block_size); + ok = AlignTo(free_area_start, logical_block_size, &free_area_start); + } + if (!ok) { + LERROR << "Integer overflow computing free area start"; + return false; } out.first_logical_sector = free_area_start / LP_SECTOR_SIZE; @@ -441,10 +450,15 @@ bool MetadataBuilder::Init(const std::vector& block_devices, // Compute the first free sector, factoring in alignment. uint64_t free_area_start = total_reserved; + bool ok; if (super.alignment || super.alignment_offset) { - free_area_start = AlignTo(free_area_start, super.alignment); + ok = AlignTo(free_area_start, super.alignment, &free_area_start); } else { - free_area_start = AlignTo(free_area_start, logical_block_size); + ok = AlignTo(free_area_start, logical_block_size, &free_area_start); + } + if (!ok) { + LERROR << "Integer overflow computing free area start"; + return false; } super.first_logical_sector = free_area_start / LP_SECTOR_SIZE; @@ -544,7 +558,11 @@ void MetadataBuilder::ExtentsToFreeList(const std::vector& extents, const Interval& current = extents[i]; DCHECK(previous.device_index == current.device_index); - uint64_t aligned = AlignSector(block_devices_[current.device_index], previous.end); + uint64_t aligned; + if (!AlignSector(block_devices_[current.device_index], previous.end, &aligned)) { + LERROR << "Sector " << previous.end << " caused integer overflow."; + continue; + } if (aligned >= current.start) { // There is no gap between these two extents, try the next one. // Note that we check with >= instead of >, since alignment may @@ -730,7 +748,10 @@ std::vector MetadataBuilder::PrioritizeSecondHalfOfSuper( // Choose an aligned sector for the midpoint. This could lead to one half // being slightly larger than the other, but this will not restrict the // size of partitions (it might lead to one extra extent if "B" overflows). - midpoint = AlignSector(super, midpoint); + if (!AlignSector(super, midpoint, &midpoint)) { + LERROR << "Unexpected integer overflow aligning midpoint " << midpoint; + return free_list; + } std::vector first_half; std::vector second_half; @@ -768,7 +789,11 @@ std::unique_ptr MetadataBuilder::ExtendFinalExtent( // If the sector ends where the next aligned chunk begins, then there's // no missing gap to try and allocate. const auto& block_device = block_devices_[extent->device_index()]; - uint64_t next_aligned_sector = AlignSector(block_device, extent->end_sector()); + uint64_t next_aligned_sector; + if (!AlignSector(block_device, extent->end_sector(), &next_aligned_sector)) { + LERROR << "Integer overflow aligning sector " << extent->end_sector(); + return nullptr; + } if (extent->end_sector() == next_aligned_sector) { return nullptr; } @@ -925,13 +950,19 @@ uint64_t MetadataBuilder::UsedSpace() const { return size; } -uint64_t MetadataBuilder::AlignSector(const LpMetadataBlockDevice& block_device, - uint64_t sector) const { +bool MetadataBuilder::AlignSector(const LpMetadataBlockDevice& block_device, uint64_t sector, + uint64_t* out) const { // Note: when reading alignment info from the Kernel, we don't assume it // is aligned to the sector size, so we round up to the nearest sector. uint64_t lba = sector * LP_SECTOR_SIZE; - uint64_t aligned = AlignTo(lba, block_device.alignment); - return AlignTo(aligned, LP_SECTOR_SIZE) / LP_SECTOR_SIZE; + if (!AlignTo(lba, block_device.alignment, out)) { + return false; + } + if (!AlignTo(*out, LP_SECTOR_SIZE, out)) { + return false; + } + *out /= LP_SECTOR_SIZE; + return true; } bool MetadataBuilder::FindBlockDeviceByName(const std::string& partition_name, @@ -1005,7 +1036,12 @@ bool MetadataBuilder::UpdateBlockDeviceInfo(size_t index, const BlockDeviceInfo& bool MetadataBuilder::ResizePartition(Partition* partition, uint64_t requested_size, const std::vector& free_region_hint) { // Align the space needed up to the nearest sector. - uint64_t aligned_size = AlignTo(requested_size, geometry_.logical_block_size); + uint64_t aligned_size; + if (!AlignTo(requested_size, geometry_.logical_block_size, &aligned_size)) { + LERROR << "Cannot resize partition " << partition->name() << " to " << requested_size + << " bytes; integer overflow."; + return false; + } uint64_t old_size = partition->size(); if (!ValidatePartitionSizeChange(partition, old_size, aligned_size, false)) { diff --git a/fs_mgr/liblp/builder_test.cpp b/fs_mgr/liblp/builder_test.cpp index 52a32175f..1a3250ae5 100644 --- a/fs_mgr/liblp/builder_test.cpp +++ b/fs_mgr/liblp/builder_test.cpp @@ -228,8 +228,9 @@ TEST_F(BuilderTest, InternalPartitionAlignment) { ASSERT_EQ(extent.target_type, LP_TARGET_TYPE_LINEAR); EXPECT_EQ(extent.num_sectors, 80); + uint64_t aligned_lba; uint64_t lba = extent.target_data * LP_SECTOR_SIZE; - uint64_t aligned_lba = AlignTo(lba, device_info.alignment); + ASSERT_TRUE(AlignTo(lba, device_info.alignment, &aligned_lba)); EXPECT_EQ(lba, aligned_lba); } @@ -1051,3 +1052,17 @@ TEST_F(BuilderTest, AlignFreeRegion) { EXPECT_EQ(e2->physical_sector(), 3072); EXPECT_EQ(e2->end_sector(), 4197368); } + +TEST_F(BuilderTest, ResizeOverflow) { + BlockDeviceInfo super("super", 8_GiB, 786432, 229376, 4096); + std::vector block_devices = {super}; + + unique_ptr builder = MetadataBuilder::New(block_devices, "super", 65536, 2); + ASSERT_NE(builder, nullptr); + + ASSERT_TRUE(builder->AddGroup("group", 0)); + + Partition* p = builder->AddPartition("system", "default", 0); + ASSERT_NE(p, nullptr); + ASSERT_FALSE(builder->ResizePartition(p, 18446744073709551615ULL)); +} diff --git a/fs_mgr/liblp/include/liblp/builder.h b/fs_mgr/liblp/include/liblp/builder.h index bd3915061..732dbeafb 100644 --- a/fs_mgr/liblp/include/liblp/builder.h +++ b/fs_mgr/liblp/include/liblp/builder.h @@ -359,7 +359,7 @@ class MetadataBuilder { bool GrowPartition(Partition* partition, uint64_t aligned_size, const std::vector& free_region_hint); void ShrinkPartition(Partition* partition, uint64_t aligned_size); - uint64_t AlignSector(const LpMetadataBlockDevice& device, uint64_t sector) const; + bool AlignSector(const LpMetadataBlockDevice& device, uint64_t sector, uint64_t* out) const; uint64_t TotalSizeOfGroup(PartitionGroup* group) const; bool UpdateBlockDeviceInfo(size_t index, const BlockDeviceInfo& info); bool FindBlockDeviceByName(const std::string& partition_name, uint32_t* index) const; diff --git a/fs_mgr/liblp/utility.h b/fs_mgr/liblp/utility.h index f210eaf7d..c4fe3edfc 100644 --- a/fs_mgr/liblp/utility.h +++ b/fs_mgr/liblp/utility.h @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -66,16 +67,26 @@ int64_t SeekFile64(int fd, int64_t offset, int whence); void SHA256(const void* data, size_t length, uint8_t out[32]); // Align |base| such that it is evenly divisible by |alignment|, which does not -// have to be a power of two. -constexpr uint64_t AlignTo(uint64_t base, uint32_t alignment) { +// have to be a power of two. Return false on overflow. +template +bool AlignTo(T base, uint32_t alignment, T* out) { + static_assert(std::numeric_limits::is_integer); + static_assert(!std::numeric_limits::is_signed); if (!alignment) { - return base; + *out = base; + return true; } - uint64_t remainder = base % alignment; + T remainder = base % alignment; if (remainder == 0) { - return base; + *out = base; + return true; } - return base + (alignment - remainder); + T to_add = alignment - remainder; + if (to_add > std::numeric_limits::max() - base) { + return false; + } + *out = base + to_add; + return true; } // Update names from C++ strings. diff --git a/fs_mgr/liblp/utility_test.cpp b/fs_mgr/liblp/utility_test.cpp index b64861d75..fc90872a5 100644 --- a/fs_mgr/liblp/utility_test.cpp +++ b/fs_mgr/liblp/utility_test.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include + #include #include #include @@ -58,15 +60,28 @@ TEST(liblp, GetMetadataOffset) { EXPECT_EQ(GetBackupMetadataOffset(geometry, 0), backup_start + 16384 * 0); } +std::optional AlignTo(uint64_t base, uint32_t alignment) { + uint64_t r; + if (!AlignTo(base, alignment, &r)) { + return {}; + } + return {r}; +} + TEST(liblp, AlignTo) { - EXPECT_EQ(AlignTo(37, 0), 37); - EXPECT_EQ(AlignTo(1024, 1024), 1024); - EXPECT_EQ(AlignTo(555, 1024), 1024); - EXPECT_EQ(AlignTo(555, 1000), 1000); - EXPECT_EQ(AlignTo(0, 1024), 0); - EXPECT_EQ(AlignTo(54, 32), 64); - EXPECT_EQ(AlignTo(32, 32), 32); - EXPECT_EQ(AlignTo(17, 32), 32); + EXPECT_EQ(AlignTo(37, 0), std::optional(37)); + EXPECT_EQ(AlignTo(1024, 1024), std::optional(1024)); + EXPECT_EQ(AlignTo(555, 1024), std::optional(1024)); + EXPECT_EQ(AlignTo(555, 1000), std::optional(1000)); + EXPECT_EQ(AlignTo(0, 1024), std::optional(0)); + EXPECT_EQ(AlignTo(54, 32), std::optional(64)); + EXPECT_EQ(AlignTo(32, 32), std::optional(32)); + EXPECT_EQ(AlignTo(17, 32), std::optional(32)); + + auto u32limit = std::numeric_limits::max(); + auto u64limit = std::numeric_limits::max(); + EXPECT_EQ(AlignTo(u64limit - u32limit + 1, u32limit), std::optional{u64limit}); + EXPECT_EQ(AlignTo(std::numeric_limits::max(), 2), std::optional{}); } TEST(liblp, GetPartitionSlotSuffix) {