From 82ac3eb14488fc682d498568390ef6786fc43d48 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Thu, 26 Jan 2023 22:14:48 +0100 Subject: [PATCH 1/3] 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 | 9 ++++++--- src/macros.rs | 6 +++--- src/tests.rs | 2 +- tests/ui/constructors-fail.rs | 4 ++-- tests/ui/sync-fail.rs | 4 ++-- 7 files changed, 26 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14c925d17..af9436c52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Made `Path::from_bytes_with_nul_unchecked` `const`. +### 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 3ca79fbaf..127f8d79d 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 7d19f8b7f..4af1865ea 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; diff --git a/src/macros.rs b/src/macros.rs index 326ab20fa..fd887d6fd 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/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, From baf44245aae1a4706f31bffe903cc70cefdb026b Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 3 Feb 2023 21:16:20 +0100 Subject: [PATCH 2/3] Replace LOOKAHEADWORDS_SIZE with LOOKAHEAD_SIZE This patch replaces the LOOKAHEADWORDS_SIZE in driver::Storage (measured in 4 bytes) with LOOKAHEAD_SIZE (measure in 8 bytes). This makes it impossible to set illegal values. --- CHANGELOG.md | 4 ++++ src/driver.rs | 7 ++----- src/fs.rs | 9 ++------- src/macros.rs | 16 ++++++++-------- src/tests.rs | 4 ++-- 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af9436c52..211f04ac2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Made `Path::from_bytes_with_nul_unchecked` `const`. +- Replaced `LOOKAHEADWORDS_SIZE` (measured in multiples of four bytes) with + `LOOKAHEAD_SIZE` (measured in multiples of eight bytes) in `driver::Storage` + so that all possible values are valid. (See the lookahead size fix below for + context.) ### Fixed - Fixed the lookahead size reported to `littlefs2-sys`. Previously, the diff --git a/src/driver.rs b/src/driver.rs index 127f8d79d..205e0c5f6 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -52,11 +52,8 @@ 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 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; + /// Size of the lookahead buffer used by littlefs, measured in multiples of 8 bytes. + type LOOKAHEAD_SIZE: ArrayLength; ///// Maximum length of a filename plus one. Stored in superblock. ///// Should default to 255+1, but associated type defaults don't exist currently. diff --git a/src/fs.rs b/src/fs.rs index 4af1865ea..666cf6fef 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -20,8 +20,7 @@ struct Cache { read: Bytes, write: Bytes, // lookahead: aligned::Aligned>, - // lookahead buffer must be aligned to 32 bytes - lookahead: generic_array::GenericArray, + lookahead: generic_array::GenericArray, } impl Cache { @@ -60,7 +59,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 = 4 * ::LOOKAHEADWORDS_SIZE::U32; + let lookahead_size: u32 = 8 * ::LOOKAHEAD_SIZE::U32; let block_cycles: i32 = Storage::BLOCK_CYCLES as _; let block_count: u32 = Storage::BLOCK_COUNT as _; @@ -88,10 +87,6 @@ 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; diff --git a/src/macros.rs b/src/macros.rs index fd887d6fd..34a55304e 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -14,7 +14,7 @@ macro_rules! ram_storage { ( cache_size_ty=$cache_size:path, block_size=$block_size:expr, block_count=$block_count:expr, - lookaheadwords_size_ty=$lookaheadwords_size:path, + lookahead_size_ty=$lookahead_size:path, filename_max_plus_one_ty=$filename_max_plus_one:path, path_max_plus_one_ty=$path_max_plus_one:path, result=$Result:ident, @@ -49,7 +49,7 @@ macro_rules! ram_storage { ( type CACHE_SIZE = $cache_size; const BLOCK_SIZE: usize = $block_size; const BLOCK_COUNT: usize = $block_count; - type LOOKAHEADWORDS_SIZE = $lookaheadwords_size; + type LOOKAHEAD_SIZE = $lookahead_size; fn read(&mut self, offset: usize, buf: &mut [u8]) -> $Result { let read_size: usize = Self::READ_SIZE; @@ -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::U2, + lookahead_size_ty=$crate::consts::U1, 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::U2, + lookahead_size_ty=$crate::consts::U1, filename_max_plus_one_ty=$crate::consts::U256, path_max_plus_one_ty=$crate::consts::U256, result=Result, @@ -127,7 +127,7 @@ macro_rules! ram_storage { ( cache_size_ty=$crate::consts::U32, block_size=256, block_count=512, - lookaheadwords_size_ty=$crate::consts::U4, + lookahead_size_ty=$crate::consts::U4, filename_max_plus_one_ty=$crate::consts::U256, path_max_plus_one_ty=$crate::consts::U256, result=Result, @@ -146,7 +146,7 @@ macro_rules! const_ram_storage { ( cache_size_ty=$cache_size:path, block_size=$block_size:expr, block_count=$block_count:expr, - lookaheadwords_size_ty=$lookaheadwords_size:path, + lookahead_size_ty=$lookahead_size:path, filename_max_plus_one_ty=$filename_max_plus_one:path, path_max_plus_one_ty=$path_max_plus_one:path, result=$Result:ident, @@ -178,7 +178,7 @@ macro_rules! const_ram_storage { ( type CACHE_SIZE = $cache_size; const BLOCK_SIZE: usize = $block_size; const BLOCK_COUNT: usize = $block_count; - type LOOKAHEADWORDS_SIZE = $lookaheadwords_size; + type LOOKAHEAD_SIZE = $lookahead_size; fn read(&mut self, offset: usize, buf: &mut [u8]) -> $Result { let read_size: usize = Self::READ_SIZE; @@ -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::U2, + lookahead_size_ty=$crate::consts::U1, 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 706255748..fb99c34b3 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::U2, + lookahead_size_ty=consts::U1, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, @@ -42,7 +42,7 @@ ram_storage!( cache_size_ty=consts::U700, block_size=20*35, block_count=32, - lookaheadwords_size_ty=consts::U16, + lookahead_size_ty=consts::U16, filename_max_plus_one_ty=consts::U256, path_max_plus_one_ty=consts::U256, result=Result, From 72f323f258197a5988433a0e3d42fc4502b03c1e Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Fri, 3 Feb 2023 22:10:25 +0100 Subject: [PATCH 3/3] Remove rust-toolchain.toml As we fixed the lookahead buffer overflow, we no longer have to pin the Rust version. Fixes https://github.com/trussed-dev/littlefs2/issues/26 Fixes https://github.com/trussed-dev/littlefs2/issues/28 --- rust-toolchain.toml | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 rust-toolchain.toml diff --git a/rust-toolchain.toml b/rust-toolchain.toml deleted file mode 100644 index e070438fa..000000000 --- a/rust-toolchain.toml +++ /dev/null @@ -1,3 +0,0 @@ -[toolchain] -channel = "1.66.1" -profile = "minimal"