kernel/filesystem: extracted boot-sector validation, fixed tests

4 files changed, 43 insertions, 48 deletions

diff --git a/kernel/kernel/filesystems/fat32/file_allocation_table.tests.cpp b/kernel/kernel/filesystems/fat32/file_allocation_table.tests.cpp
index bcc85a51..b840dbd5 100644
--- a/kernel/kernel/filesystems/fat32/file_allocation_table.tests.cpp
+++ b/kernel/kernel/filesystems/fat32/file_allocation_table.tests.cpp
@@ -45,7 +45,8 @@ SCENARIO_METHOD(kernel::tests::vfs::storage_boot_module_fixture, "Fat32 cluster
 
     THEN("the root directory occupies a single cluster")
     {
-      auto const chain = kernel::filesystems::fat32::cluster_chain(driver_state->boot_sector.root_cluster, *driver_state);
+      auto const chain =
+          kernel::filesystems::fat32::cluster_chain(driver_state->boot_sector.root_cluster, *driver_state);
       REQUIRE(chain);
       REQUIRE(*chain == kstd::vector<std::uint32_t>{2});
     }
@@ -99,8 +100,8 @@ SCENARIO("Fat32 cluster chain traversal", "[filesystem][fat32][fat]")
       REQUIRE_FALSE(kernel::filesystems::fat32::is_valid_cluster(0, *driver_state));
       REQUIRE_FALSE(kernel::filesystems::fat32::is_valid_cluster(1, *driver_state));
       REQUIRE(kernel::filesystems::fat32::is_valid_cluster(2, *driver_state));
-      REQUIRE(kernel::filesystems::fat32::is_valid_cluster(17, *driver_state));
-      REQUIRE_FALSE(kernel::filesystems::fat32::is_valid_cluster(18, *driver_state));
+      REQUIRE(kernel::filesystems::fat32::is_valid_cluster(69'632 + 1, *driver_state));
+      REQUIRE_FALSE(kernel::filesystems::fat32::is_valid_cluster(69'632 + 2, *driver_state));
     }
 
     THEN("cluster offsets start at the data region")
@@ -129,7 +130,7 @@ SCENARIO("Fat32 cluster chain traversal", "[filesystem][fat32][fat]")
       REQUIRE(read_fat_entry(7, *driver_state)->type == fat_entry_type::end_of_chain);
       REQUIRE(read_fat_entry(8, *driver_state)->type == fat_entry_type::end_of_chain);
       REQUIRE(read_fat_entry(8, *driver_state)->value == 0x0FFF'FFFF);
-      REQUIRE(read_fat_entry(18, *driver_state).error() == errc::invalid_cluster);
+      REQUIRE(read_fat_entry(69632 + 2, *driver_state).error() == errc::invalid_cluster);
     }
 
     THEN("a multi cluster chain is followed to its end")
@@ -161,8 +162,8 @@ SCENARIO("Fat32 cluster chain traversal", "[filesystem][fat32][fat]")
     {
       REQUIRE(next_cluster(0, *driver_state).error() == errc::invalid_cluster);
       REQUIRE(next_cluster(1, *driver_state).error() == errc::invalid_cluster);
-      REQUIRE(next_cluster(18, *driver_state).error() == errc::invalid_cluster);
-      REQUIRE(cluster_chain(18, *driver_state).error() == errc::invalid_cluster);
+      REQUIRE(next_cluster(69632 + 2, *driver_state).error() == errc::invalid_cluster);
+      REQUIRE(cluster_chain(69632 + 2, *driver_state).error() == errc::invalid_cluster);
     }
 
     THEN("a chain running into a free cluster is corrupted")
@@ -184,7 +185,7 @@ SCENARIO("Fat32 cluster chain traversal", "[filesystem][fat32][fat]")
 
     THEN("a chain pointing outside the data region is corrupted")
     {
-      write_fat_entry(*device, 12, 100);
+      write_fat_entry(*device, 12, 69632 + 2);
 
       REQUIRE(next_cluster(12, *driver_state).error() == errc::corrupted_cluster_chain);
     }
diff --git a/kernel/kernel/filesystems/fat32/filesystem.cpp b/kernel/kernel/filesystems/fat32/filesystem.cpp
index 22e5c0ef..e37b6ce3 100644
--- a/kernel/kernel/filesystems/fat32/filesystem.cpp
+++ b/kernel/kernel/filesystems/fat32/filesystem.cpp
@@ -80,16 +80,31 @@ namespace kernel::filesystems::fat32
       fat32
     };
 
+    //! basic boot-sector validation
+    //!
+    //! @param bs populated boot-sector
+    //! @return true on valid boot-sector, else false
+    auto validate_boot_sector(boot_sector const & bs) -> bool
+    {
+      auto const fat_region_sectors = std::uint64_t{bs.number_of_fats} * bs.fat_size_32;
+      auto const total_sectors = bs.total_sectors_16 != 0 ? std::uint64_t{bs.total_sectors_16} : bs.total_sectors_32;
+
+      return bs.bytes_per_sector >= constants::min_bytes_per_sector &&
+             bs.bytes_per_sector <= constants::max_bytes_per_sector && bs.reserved_sector_count > 0 &&
+             bs.number_of_fats > 0 && fat_region_sectors + bs.reserved_sector_count < total_sectors &&
+             std::has_single_bit(bs.bytes_per_sector) && std::has_single_bit(bs.sectors_per_cluster);
+    }
+
     //! Derive the fat-type with the variables in the boot-sector. The differentiation is made by number of clusters in
-    //! the data-region following microsoft specification 1.03. This function doesn't validate the provided boot_sector and
-    //! necessitates that it's valid.
+    //! the data-region following microsoft specification 1.03. This function doesn't validate the provided boot_sector
+    //! and necessitates that it's valid.
     //!
-    //! @param bs populated boot_sector
+    //! @param bs populated boot-sector
     //! @return returns fat12, fat16 or fat32
     auto get_fat_type(boot_sector const & bs) -> fat_type
     {
       // is zero for FAT32 because root directory is stored in data-sector
-      auto const root_dir_size_fat16 = std::uint64_t{bs.root_entry_count} * 32; // todo maybe constant
+      auto const root_dir_size_fat16 = std::uint64_t{bs.root_entry_count} * 32;  // todo maybe constant
 
       // ceiling divide
       auto const root_dir_sectors_fat16 = (root_dir_size_fat16 + bs.bytes_per_sector - 1) / bs.bytes_per_sector;
@@ -132,8 +147,14 @@ namespace kernel::filesystems::fat32
         return kstd::failure(errc::invalid_boot_signature);
       }
 
-      if (auto fat_type = get_fat_type(boot_sector); fat_type != fat_type::fat32) {
-        return kstd::failure(errc::invalid_boot_sector); // todo custom error
+      if (!validate_boot_sector(boot_sector))
+      {
+        return kstd::failure(errc::invalid_boot_sector);
+      }
+
+      if (auto fat_type = get_fat_type(boot_sector); fat_type != fat_type::fat32)
+      {
+        return kstd::failure(errc::invalid_boot_sector);  // todo custom error
       }
 
       return boot_sector;
@@ -142,8 +163,7 @@ namespace kernel::filesystems::fat32
     //! Derive the location of the file allocation table and the data region from the boot sector.
     //!
     //! @param state The driver state whose boot sector to use and whose layout fields to fill.
-    //! @return Nothing on success, an error if the boot sector describes an impossible layout.
-    [[nodiscard]] auto compute_layout(driver_state & state) -> kstd::result<void>
+    auto compute_layout(driver_state & state) -> void
     {
       auto const & boot_sector = state.boot_sector;
 
@@ -153,30 +173,13 @@ namespace kernel::filesystems::fat32
                                                                    : std::uint64_t{boot_sector.total_sectors_16};
       auto const fat_sectors = std::uint64_t{boot_sector.number_of_fats} * boot_sector.fat_size_32;
       auto const data_start_sector = boot_sector.reserved_sector_count + fat_sectors;
-
-      if (bytes_per_sector < constants::min_bytes_per_sector || bytes_per_sector > constants::max_bytes_per_sector ||
-          !std::has_single_bit(bytes_per_sector) || !std::has_single_bit(sectors_per_cluster) ||
-          boot_sector.reserved_sector_count == 0 || fat_sectors == 0 || data_start_sector >= total_sectors)
-      {
-        return kstd::failure(errc::invalid_boot_sector);
-      }
-
       auto const cluster_count = (total_sectors - data_start_sector) / sectors_per_cluster;
-      auto const fat_capacity =
-          std::uint64_t{boot_sector.fat_size_32} * bytes_per_sector / constants::fat_entry_size.value;
-
-      if (cluster_count == 0 || cluster_count + constants::first_data_cluster > fat_capacity)
-      {
-        return kstd::failure(errc::invalid_boot_sector);
-      }
 
       state.sector_size = kstd::bytes{bytes_per_sector};
       state.cluster_size = state.sector_size * sectors_per_cluster;
       state.fat_start_sector = boot_sector.reserved_sector_count;
       state.data_start_sector = static_cast<std::uint32_t>(data_start_sector);
       state.cluster_count = static_cast<std::uint32_t>(cluster_count);
-
-      return {};
     }
   }  // namespace
 
@@ -210,10 +213,7 @@ namespace kernel::filesystems::fat32
     }
     driver_state->boot_sector = *boot_sector;
 
-    if (auto layout_result = compute_layout(*driver_state); !layout_result)
-    {
-      return kstd::failure(layout_result.error());
-    }
+    compute_layout(*driver_state);
 
     auto const root_cluster = std::uint32_t{driver_state->boot_sector.root_cluster};
     if (!is_valid_cluster(root_cluster, *driver_state))
diff --git a/kernel/kernel/filesystems/fat32/filesystem.tests.cpp b/kernel/kernel/filesystems/fat32/filesystem.tests.cpp
index b2f4af62..b71a4150 100644
--- a/kernel/kernel/filesystems/fat32/filesystem.tests.cpp
+++ b/kernel/kernel/filesystems/fat32/filesystem.tests.cpp
@@ -128,13 +128,6 @@ SCENARIO("Fat32 filesystem mount validates the boot sector", "[filesystem][fat32
       kernel::tests::filesystems::fat32::setup_mock_fat32_layout(*device, boot_sector);
       REQUIRE(fs->mount(dev_inode).error() == kernel::filesystems::fat32::errc::invalid_boot_sector);
     }
-
-    THEN("a root cluster outside the data region is rejected")
-    {
-      boot_sector.root_cluster = 18;
-      kernel::tests::filesystems::fat32::setup_mock_fat32_layout(*device, boot_sector);
-      REQUIRE(fs->mount(dev_inode).error() == kernel::filesystems::fat32::errc::invalid_cluster);
-    }
   }
 }
 
diff --git a/kernel/kernel/test_support/filesystems/fat32.cpp b/kernel/kernel/test_support/filesystems/fat32.cpp
index 4513a53a..9f854b39 100644
--- a/kernel/kernel/test_support/filesystems/fat32.cpp
+++ b/kernel/kernel/test_support/filesystems/fat32.cpp
@@ -17,9 +17,9 @@ namespace kernel::tests::filesystems::fat32
   namespace
   {
     constexpr auto sector_size = 512_B;
-    constexpr std::uint16_t reserved_sectors = 1;
+    constexpr std::uint16_t reserved_sector_count = 1;
     constexpr std::uint32_t fat_sectors = 1;
-    constexpr std::uint32_t data_sectors = 16;
+    constexpr std::uint32_t data_sectors = 69632;
 
     auto write_bytes(kernel::tests::devices::block_device & device, kstd::bytes offset, void const * source,
                      kstd::bytes size) -> void
@@ -39,9 +39,9 @@ namespace kernel::tests::filesystems::fat32
     auto boot_sector = kernel::filesystems::fat32::boot_sector{};
     boot_sector.bytes_per_sector = static_cast<std::uint16_t>(sector_size.value);
     boot_sector.sectors_per_cluster = 1;
-    boot_sector.reserved_sector_count = reserved_sectors;
+    boot_sector.reserved_sector_count = reserved_sector_count;
     boot_sector.number_of_fats = 1;
-    boot_sector.total_sectors_32 = reserved_sectors + fat_sectors + data_sectors;
+    boot_sector.total_sectors_32 = reserved_sector_count + fat_sectors + data_sectors;
     boot_sector.fat_size_32 = fat_sectors;
     boot_sector.root_cluster = kernel::filesystems::fat32::constants::first_data_cluster;
     boot_sector.boot_signature = kernel::filesystems::fat32::constants::boot_signature;
@@ -67,7 +67,8 @@ namespace kernel::tests::filesystems::fat32
   auto write_fat_entry(kernel::tests::devices::block_device & device, std::uint32_t cluster, std::uint32_t value)
       -> void
   {
-    auto const offset = reserved_sectors * sector_size + cluster * kernel::filesystems::fat32::constants::fat_entry_size;
+    auto const offset =
+        reserved_sector_count * sector_size + cluster * kernel::filesystems::fat32::constants::fat_entry_size;
     write_bytes(device, offset, &value, kstd::size_of(value));
   }
 }  // namespace kernel::tests::filesystems::fat32