From 40d68bb7b534f36bfde548b55582189db64ed9c1 Mon Sep 17 00:00:00 2001 From: Tomek Sobkowiak Date: Fri, 29 Jan 2021 13:00:06 +0100 Subject: [PATCH] [EGD-4495] Check partitions during registering new disk image Check partitions validity when registering disk. Add unittest and test disk image generation. --- .../purefs/blkdev/partition_parser.hpp | 4 ++ .../src/purefs/blkdev/partition_parser.cpp | 38 +++++++++++--- module-vfs/tests/CMakeLists.txt | 12 ++++- module-vfs/tests/gendisktestimg.sh | 41 ++++++++++++++++ module-vfs/tests/unittest_disk_manager.cpp | 49 +++++++++++++++++-- 5 files changed, 130 insertions(+), 14 deletions(-) create mode 100755 module-vfs/tests/gendisktestimg.sh diff --git a/module-vfs/include/internal/purefs/blkdev/partition_parser.hpp b/module-vfs/include/internal/purefs/blkdev/partition_parser.hpp index 4c2b0b8ac53350435d05f682fe450dbbd82b96cb..759bb67242e159812d9ba6551e7aaf2c967d7d89 100644 --- a/module-vfs/include/internal/purefs/blkdev/partition_parser.hpp +++ b/module-vfs/include/internal/purefs/blkdev/partition_parser.hpp @@ -2,6 +2,9 @@ // For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md #pragma once + +#include + #include #include #include @@ -25,6 +28,7 @@ namespace purefs::blkdev static auto read_partitions(const std::vector &buffer, std::array &parts) -> void; static auto is_extended(uint8_t type) -> bool; auto parse_extended(uint32_t lba, uint32_t count) -> int; + auto check_partition(const std::shared_ptr disk, const partition &part) -> bool; private: const std::shared_ptr m_disk; diff --git a/module-vfs/src/purefs/blkdev/partition_parser.cpp b/module-vfs/src/purefs/blkdev/partition_parser.cpp index c2d0022f6ac0b54c62e7fd61d3a2587fbd47d208..4a75aca51f68bb56bd2619223c7ab9bce099b67e 100644 --- a/module-vfs/src/purefs/blkdev/partition_parser.cpp +++ b/module-vfs/src/purefs/blkdev/partition_parser.cpp @@ -71,6 +71,10 @@ namespace purefs::blkdev::internal for (auto &part : root_part) { if (is_extended(part.type)) continue; + + if (!check_partition(m_disk, part)) + continue; + if (part.num_sectors) { part.physical_number = part_no; part.mbr_number = part_no; @@ -88,6 +92,26 @@ namespace purefs::blkdev::internal return ret; } + auto partition_parser::check_partition(const std::shared_ptr disk, const partition &part) -> bool + { + auto sector_size = disk->get_info(info_type::sector_size); + unsigned long this_size = disk->get_info(info_type::sector_count) * sector_size; + const auto poffset = part.start_sector * sector_size; + const auto psize = part.num_sectors * sector_size; + const auto pnext = part.start_sector * sector_size + poffset; + if ((poffset + psize > this_size) || // oversized + (pnext < static_cast(part.start_sector * sector_size)) // going backward + ) { + LOG_WARN("Part %d looks strange: start_sector %u offset %u next %u\n", + unsigned(part.mbr_number), + unsigned(part.start_sector), + unsigned(poffset), + unsigned(pnext)); + return false; + } + return true; + } + auto partition_parser::read_partitions(const std::vector &buffer, std::array &parts) -> void { @@ -112,8 +136,8 @@ namespace purefs::blkdev::internal auto sector_size = m_disk->get_info(info_type::sector_size); int extended_part_num; std::array parts; - auto current_sector = lba; - auto this_size = count; + auto current_sector = lba; + unsigned long this_size = count * sector_size; if (sector_size < 0) { return sector_size; } @@ -156,10 +180,10 @@ namespace purefs::blkdev::internal /* Some sanity checks */ const auto poffset = parts[partition_num].start_sector * sector_size; const auto psize = parts[partition_num].num_sectors * sector_size; - const auto pnext = current_sector + poffset; - if ((poffset + psize > this_size) || // oversized - (pnext < lba) || // going backward - (pnext > lba + count) // outsized + const auto pnext = current_sector * sector_size + poffset; + if ((poffset + psize > this_size) || // oversized + (pnext < static_cast(lba * sector_size)) || // going backward + (pnext > static_cast((lba + count) * sector_size)) // outsized ) { LOG_WARN("Part %d looks strange: current_sector %u offset %u next %u\n", int(partition_num), @@ -179,7 +203,7 @@ namespace purefs::blkdev::internal break; /* nothing left to do */ } /* Examine the next extended partition */ - current_sector = lba + parts[extended_part_num].start_sector * sector_size; + current_sector = lba + parts[extended_part_num].start_sector; this_size = parts[extended_part_num].num_sectors * sector_size; } return error; diff --git a/module-vfs/tests/CMakeLists.txt b/module-vfs/tests/CMakeLists.txt index cd779a71e626f124ca7ef184170500a2a425cadd..e6ccd9f047f1fc1a7d664d9aaff66666c898ece4 100644 --- a/module-vfs/tests/CMakeLists.txt +++ b/module-vfs/tests/CMakeLists.txt @@ -1,11 +1,21 @@ cmake_minimum_required(VERSION 3.12) +add_custom_target("test_disk_image") +add_custom_command( + PRE_BUILD + TARGET test_disk_image + COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/gendisktestimg.sh ${CMAKE_BINARY_DIR} + WORKING_DIRECTORY ${CMAKE_BINARY_DIR} +) + add_catch2_executable( NAME vfs-disk SRCS ${CMAKE_CURRENT_LIST_DIR}/unittest_disk_manager.cpp LIBS module-vfs + DEPS + test_disk_image ) add_catch2_executable( @@ -17,7 +27,6 @@ add_catch2_executable( ) set(LITTLEFS_IMAGE "lfstest.img") - add_custom_command( OUTPUT ${LITTLEFS_IMAGE} COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/genlfstestimg.sh 1G ${LITTLEFS_IMAGE} @@ -26,7 +35,6 @@ add_custom_command( ) add_custom_target( test_lfs_image DEPENDS ${LITTLEFS_IMAGE} ) - add_catch2_executable( NAME vfs-littlefs SRCS diff --git a/module-vfs/tests/gendisktestimg.sh b/module-vfs/tests/gendisktestimg.sh new file mode 100755 index 0000000000000000000000000000000000000000..bb69253821e27a5cd98e3a1d84d409043e3b9748 --- /dev/null +++ b/module-vfs/tests/gendisktestimg.sh @@ -0,0 +1,41 @@ +#!/bin/bash -e +# Copyright (c) 2017-2021, Mudita Sp. z.o.o. All rights reserved. +# For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md + +#Copyright (c) 2017-2020, Mudita Sp. z.o.o. All rights reserved. +#For licensing, see https://github.com/mudita/MuditaOS/LICENSE.md + +if [ $# -ne 1 ]; then + echo "Error! Invalid argument count" + exit -1 +fi +IMAGE_FILE_DIR="$1" + +dd if=/dev/zero of=$IMAGE_FILE_DIR/test_disk.img bs=512 count=8 +sfdisk $IMAGE_FILE_DIR/test_disk.img << ==sfdisk +,1,L +,1,L +,,L +==sfdisk + +dd if=/dev/zero of=$IMAGE_FILE_DIR/test_disk_ext.img bs=512 count=16 +sfdisk $IMAGE_FILE_DIR/test_disk_ext.img << ==sfdisk +,1,L +,,E +,1,L +,1,L +,1,L +,1,L +,1,L +,,L +==sfdisk + +dd if=/dev/zero of=$IMAGE_FILE_DIR/test_disk_bad_tmp.img bs=512 count=16 +sfdisk $IMAGE_FILE_DIR/test_disk_bad_tmp.img << ==sfdisk +,1,L +==sfdisk + +# Modify parition table to simply broke it, which ten can be tested. +{ head -c 458 $IMAGE_FILE_DIR/test_disk_bad_tmp.img; printf '\xFF'; tail -c +460 $IMAGE_FILE_DIR/test_disk_bad_tmp.img; } > $IMAGE_FILE_DIR/test_disk_bad.img +rm $IMAGE_FILE_DIR/test_disk_bad_tmp.img + diff --git a/module-vfs/tests/unittest_disk_manager.cpp b/module-vfs/tests/unittest_disk_manager.cpp index e2aa0b599b30b770b6be89d65564a6bcac4b640e..cb1823d47c4ef4ecfd663c9280c6ff7a19f92720 100644 --- a/module-vfs/tests/unittest_disk_manager.cpp +++ b/module-vfs/tests/unittest_disk_manager.cpp @@ -7,8 +7,11 @@ namespace { - constexpr auto disk_image = "PurePhone.img"; -} + constexpr auto disk_image = "PurePhone.img"; + constexpr auto part_disk_image = "test_disk.img"; + constexpr auto part_disk_image_ext = "test_disk_ext.img"; + constexpr auto part_disk_image_bad = "test_disk_bad.img"; +} // namespace TEST_CASE("Registering and unregistering device") { using namespace purefs; @@ -29,16 +32,16 @@ TEST_CASE("Parsing and checking partititons") { using namespace purefs; blkdev::disk_manager dm; - auto disk = std::make_shared(disk_image); + auto disk = std::make_shared(part_disk_image); REQUIRE(disk); REQUIRE(dm.register_device(disk, "emmc0") == 0); const auto parts = dm.partitions("emmc0"); - REQUIRE(parts.size() > 1); + REQUIRE(parts.size() == 3); unsigned long prev_start = 0; int num{}; for (const auto &part : parts) { REQUIRE(part.physical_number > 0); - REQUIRE(part.start_sector >= 2048); + REQUIRE(part.start_sector > 0); REQUIRE(part.num_sectors > 0); REQUIRE(part.start_sector >= prev_start); REQUIRE(part.type > 0); @@ -47,6 +50,42 @@ TEST_CASE("Parsing and checking partititons") } } +TEST_CASE("Parsing and checking extended partititons") +{ + using namespace purefs; + blkdev::disk_manager dm; + auto disk = std::make_shared(part_disk_image_ext); + REQUIRE(disk); + REQUIRE(dm.register_device(disk, "emmc0") == 0); + const auto parts = dm.partitions("emmc0"); + + REQUIRE(parts.size() == 7); + unsigned long prev_start = 0; + int num{}; + for (const auto &part : parts) { + REQUIRE(part.physical_number > 0); + REQUIRE(part.start_sector >= 1); + REQUIRE(part.num_sectors > 0); + REQUIRE(part.num_sectors < 0xFF); + REQUIRE(part.start_sector >= prev_start); + REQUIRE(part.type > 0); + REQUIRE(part.name == ("emmc0part" + std::to_string(num++))); + prev_start = part.num_sectors + part.start_sector; + } +} + +TEST_CASE("Parsing and checking invalid partititons") +{ + using namespace purefs; + blkdev::disk_manager dm; + auto disk = std::make_shared(part_disk_image_bad); + REQUIRE(disk); + REQUIRE(dm.register_device(disk, "emmc0") == 0); + const auto parts = dm.partitions("emmc0"); + + REQUIRE(parts.size() == 0); +} + TEST_CASE("RW boundary checking") { using namespace purefs;