From 1a0b6491efb634f7a735fe7aef77250b9f94ffec Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Thu, 26 Jan 2023 22:14:48 +0100 Subject: [PATCH] Fix lookahead buffer size reported to littlefs2-sys Previously, we reported the lookahead buffer size in bytes but littlefs2-sys expects the lookahead buffer size as a multiple of 8 bytes. This could lead to a buffer overflow causing filesystem corruption. This patch fixes the reported lookahead buffer size. Note that Storage::LOOKAHEAD_WORDS_SIZE allows users to set invalid values (as it is measured in 4 bytes, not in 8 bytes). Invalid values that were previously accepted because of the wrong buffer size calculation can now be rejected by littlefs2-sys. This is a combination of two previous patches: https://github.com/trussed-dev/littlefs2/pull/19 https://github.com/Nitrokey/littlefs2/pull/1 Fixes: https://github.com/trussed-dev/littlefs2/issues/16 --- CHANGELOG.md | 9 +++++++++ src/driver.rs | 7 +++---- src/fs.rs | 10 ++++++---- src/macros.rs | 6 +++--- src/tests.rs | 2 +- tests/test_serde.rs | 1 - tests/ui/constructors-fail.rs | 4 ++-- tests/ui/sync-fail.rs | 4 ++-- 8 files changed, 26 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5001608ee..b64d2f814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] +### Fixed +- Fixed the lookahead size reported to `littlefs2-sys`. Previously, the + reported size was too large by the factor of 8, potentially leading to a + buffer overflow causing filesystem corruption. Fixing this means that + `Storage::LOOKAHEADWORD_SIZE` values that are not a multiple of 2 can now + lead to an error. Fixes [#16]. + +[#16]: https://github.com/trussed-dev/littlefs2/issues/16 + ## [v0.2.2] - 2021-03-20 ### Changed diff --git a/src/driver.rs b/src/driver.rs index acd19704d..d89f6f46c 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -52,10 +52,9 @@ pub trait Storage { /// Must be a factor of `BLOCK_SIZE`. type CACHE_SIZE: ArrayLength; - /// littlefs itself has a `LOOKAHEAD_SIZE`, which must be a multiple of 8, - /// as it stores data in a bitmap. It also asks for 4-byte aligned buffers. - /// Hence, we further restrict `LOOKAHEAD_SIZE` to be a multiple of 32. - /// Our LOOKAHEADWORDS_SIZE is this multiple. + /// littlefs itself has a `LOOKAHEAD_SIZE`, which must be a multiple of 8 bytes. For + /// historical reasons, `LOOKAHEADWORDS_SIZE` is measured in 4 bytes. This means that users + /// must always provide a multiple of 2 here. type LOOKAHEADWORDS_SIZE: ArrayLength; // type LOOKAHEAD_SIZE: ArrayLength; diff --git a/src/fs.rs b/src/fs.rs index f40631292..18ea3b92b 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -20,6 +20,7 @@ struct Cache { read: Bytes, write: Bytes, // lookahead: aligned::Aligned>, + // lookahead buffer must be aligned to 32 bytes lookahead: generic_array::GenericArray, } @@ -28,7 +29,6 @@ impl Cache { Self { read: Default::default(), write: Default::default(), - // lookahead: aligned::Aligned(Default::default()), lookahead: Default::default(), } } @@ -60,8 +60,7 @@ impl Allocation { let write_size: u32 = Storage::WRITE_SIZE as _; let block_size: u32 = Storage::BLOCK_SIZE as _; let cache_size: u32 = ::CACHE_SIZE::U32; - let lookahead_size: u32 = - 32 * ::LOOKAHEADWORDS_SIZE::U32; + let lookahead_size: u32 = 4 * ::LOOKAHEADWORDS_SIZE::U32; let block_cycles: i32 = Storage::BLOCK_CYCLES as _; let block_count: u32 = Storage::BLOCK_COUNT as _; @@ -89,6 +88,10 @@ impl Allocation { debug_assert!(cache_size <= block_size); debug_assert!(block_size % cache_size == 0); + // lookahead words size (measured in 4 bytes) must be a multiple of 2 so that the actual + // lookahead size is a multiple of 8 bytes + debug_assert!(lookahead_size % 2 == 0); + let cache = Cache::new(); let filename_max_plus_one: u32 = crate::consts::FILENAME_MAX_PLUS_ONE; @@ -1311,7 +1314,6 @@ impl<'a, Storage: driver::Storage> Filesystem<'a, Storage> { mod tests { use super::*; use core::convert::TryInto; - use generic_array::typenum::consts; use driver::Storage as LfsStorage; use io::Result as LfsResult; const_ram_storage!(TestStorage, 4096); diff --git a/src/macros.rs b/src/macros.rs index 6d4fd6a5b..05c15b281 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -93,7 +93,7 @@ macro_rules! ram_storage { ( cache_size_ty=$crate::consts::U32, block_size=128, block_count=$bytes/128, - lookaheadwords_size_ty=$crate::consts::U1, + lookaheadwords_size_ty=$crate::consts::U2, filename_max_plus_one_ty=$crate::consts::U256, path_max_plus_one_ty=$crate::consts::U256, result=LfsResult, @@ -110,7 +110,7 @@ macro_rules! ram_storage { ( cache_size_ty=$crate::consts::U32, block_size=128, block_count=8, - lookaheadwords_size_ty=$crate::consts::U1, + lookaheadwords_size_ty=$crate::consts::U2, filename_max_plus_one_ty=$crate::consts::U256, path_max_plus_one_ty=$crate::consts::U256, result=Result, @@ -221,7 +221,7 @@ macro_rules! const_ram_storage { ( cache_size_ty=$crate::consts::U512, block_size=512, block_count=$bytes/512, - lookaheadwords_size_ty=$crate::consts::U1, + lookaheadwords_size_ty=$crate::consts::U2, filename_max_plus_one_ty=$crate::consts::U256, path_max_plus_one_ty=$crate::consts::U256, result=LfsResult, diff --git a/src/tests.rs b/src/tests.rs index 8dae28647..706255748 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -26,7 +26,7 @@ ram_storage!( cache_size_ty=consts::U32, block_size=256, block_count=512, - lookaheadwords_size_ty=consts::U1, + lookaheadwords_size_ty=consts::U2, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, diff --git a/tests/test_serde.rs b/tests/test_serde.rs index da72d228d..21b11200e 100644 --- a/tests/test_serde.rs +++ b/tests/test_serde.rs @@ -1,5 +1,4 @@ use littlefs2::{ - consts, driver, fs::Filesystem, io::Result, diff --git a/tests/ui/constructors-fail.rs b/tests/ui/constructors-fail.rs index b44b5ff36..ebc701e1c 100644 --- a/tests/ui/constructors-fail.rs +++ b/tests/ui/constructors-fail.rs @@ -15,7 +15,7 @@ ram_storage!( cache_size_ty=consts::U32, block_size=256, block_count=512, - lookaheadwords_size_ty=consts::U1, + lookaheadwords_size_ty=consts::U2, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, @@ -31,7 +31,7 @@ ram_storage!( cache_size_ty=consts::U700, block_size=20*35, block_count=32, - lookaheadwords_size_ty=consts::U1, + lookaheadwords_size_ty=consts::U2, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, diff --git a/tests/ui/sync-fail.rs b/tests/ui/sync-fail.rs index cc1a0131d..3954abf1a 100644 --- a/tests/ui/sync-fail.rs +++ b/tests/ui/sync-fail.rs @@ -15,7 +15,7 @@ ram_storage!( cache_size_ty=consts::U32, block_size=256, block_count=512, - lookaheadwords_size_ty=consts::U1, + lookaheadwords_size_ty=consts::U2, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, @@ -31,7 +31,7 @@ ram_storage!( cache_size_ty=consts::U700, block_size=20*35, block_count=32, - lookaheadwords_size_ty=consts::U1, + lookaheadwords_size_ty=consts::U2, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result,