Fix lookahead buffer size reported to littlefs2-sys

[robin-nitrokey]

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:
#19
Nitrokey#1

Fixes: #16

---

I’m wondering whether we should treat this as a breaking change or not. On the one hand, this is a bug fix. On the other hand, it may cause code that previously worked (because the size calculation was wrong) to fail. If we consider this to be a breaking change, we should also change Storage::LOOKAHEAD_BUFFER_SIZE so that it uses multiples of 8 and it is no longer possible to set invalid values.

Also, it would be good to have a test case that demonstrates the bug.

cc @arturkow2000

[daringer]

lgtm, this is something we should verify on actual hardware, just to be sure and as finding this as a source for issues at a later point might be painful

[robin-nitrokey]

Note that this is effectively the same patch that we are already using in nitrokey-3-firmware.

[daringer]

just the usual inner panic on my side once something touches littlefs2, but you're right, this should be fine.

[nickray]

I'd be +1 on making this a breaking change, and adding whatever other improvements you can think of (e.g. your "should also change").

[robin-nitrokey]

Updated:

• Rebased onto 7b66857.
• Replaced LOOKAHEADWORDS_SIZE (measured in 4 bytes) with LOOKAHEAD_SIZE (measured in 8 bytes) so that all possible values are valid.
• Bumped Rust version back to 1.67.0.

Not tested on hardware yet, also no unit test for the issue yet. But at least the 1.67.0 segfault fix shows us that we are doing something right. :-)

I’d like to merge and release this next week as this a rather severe issue.

@trussed-dev/nitrokey Please review, test and comment.
@daringer Can you test this on the NK3AM with your load test script?
@arturkow2000 @lexxvir @svenstaro If you’re still working with littlefs2, would you mind testing this change in your projects?

[robin-nitrokey]

Forgot about this one, has to be removed as we now use multiples of 8 bytes.

[nickray]

I'm wondering if we should have all the sizes (at least external facing) in bytes, and use compile time checks to enforce multiples, and compile time additions for the "plus one" stuff -- if possible -- as it would make for a cleaner API.

[robin-nitrokey]

I don’t think this is possible at the moment because of the limitations with the const generics in stable. We cannot use generic parameters in const operations, so we still need generic-array which cannot do arithmetics on the array size AFAIK. And we cannot use generic parameters in const assertions.

I thought about adding an optional verification method that could be used to validate a Storage implementation, but it is nothing we can enforce. So I’d prefer to stick with the current approach for compile-time checks.

[sosthene-nitrokey]

And we cannot use generic parameters in const assertions.

This can be done by having an associated constant for the const assertion: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40f243447104f266f22aa71b3f18c359

[robin-nitrokey]

In this case, it is about accessing an associated constant of a generic parameter. Moving all associated constant into parameters is unrealistic.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4e53f8caa18adb02be25667bbf27d3bc

littlefs2/src/driver.rs

Lines 25 to 103 in 7b66857

	pub trait Storage {
	// /// Error type for user-provided read/write/erase methods
	// type Error = usize;
	/// Minimum size of block read in bytes. Not in superblock
	const READ_SIZE: usize;
	/// Minimum size of block write in bytes. Not in superblock
	const WRITE_SIZE: usize;
	/// Size of an erasable block in bytes, as unsigned typenum.
	/// Must be a multiple of both `READ_SIZE` and `WRITE_SIZE`.
	/// At least 128 (https://git.io/JeHp9). Stored in superblock.
	const BLOCK_SIZE: usize;
	/// Number of erasable blocks.
	/// Hence storage capacity is `BLOCK_COUNT * BLOCK_SIZE`
	const BLOCK_COUNT: usize;
	/// Suggested values are 100-1000, higher is more performant but
	/// less wear-leveled. Default of -1 disables wear-leveling.
	/// Value zero is invalid, must be positive or -1.
	const BLOCK_CYCLES: isize = -1;
	/// littlefs uses a read cache, a write cache, and one cache per per file.
	/// Must be a multiple of `READ_SIZE` and `WRITE_SIZE`.
	/// Must be a factor of `BLOCK_SIZE`.
	type CACHE_SIZE: ArrayLength<u8>;
	/// 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.
	type LOOKAHEADWORDS_SIZE: ArrayLength<u32>;
	// type LOOKAHEAD_SIZE: ArrayLength<u8>;
	///// Maximum length of a filename plus one. Stored in superblock.
	///// Should default to 255+1, but associated type defaults don't exist currently.
	///// At most 1_022+1.
	/////
	///// TODO: We can't actually change this - need to pass on as compile flag
	///// to the C backend.
	//type FILENAME_MAX_PLUS_ONE: ArrayLength<u8>;
	// /// Maximum length of a path plus one. Necessary to convert Rust string slices
	// /// to C strings, which requires an allocation for the terminating
	// /// zero-byte. If in doubt, set to `FILENAME_MAX_PLUS_ONE`.
	// /// Must be larger than `FILENAME_MAX_PLUS_ONE`.
	// type PATH_MAX_PLUS_ONE: ArrayLength<u8>;
	///// Maximum size of file. Stored in superblock.
	///// Defaults to 2_147_483_647 (or u31, to avoid sign issues in the C code).
	///// At most 2_147_483_647.
	/////
	///// TODO: We can't actually change this - need to pass on as compile flag
	///// to the C backend.
	//const FILEBYTES_MAX: usize = ll::LFS_FILE_MAX as _;
	///// Maximum size of custom attributes.
	///// Should default to 1_022, but associated type defaults don't exists currently.
	///// At most 1_022.
	/////
	///// TODO: We can't actually change this - need to pass on as compile flag
	///// to the C backend.
	//type ATTRBYTES_MAX: ArrayLength<u8>;
	/// Read data from the storage device.
	/// Guaranteed to be called only with bufs of length a multiple of READ_SIZE.
	fn read(&mut self, off: usize, buf: &mut [u8]) -> Result<usize>;
	/// Write data to the storage device.
	/// Guaranteed to be called only with bufs of length a multiple of WRITE_SIZE.
	fn write(&mut self, off: usize, data: &[u8]) -> Result<usize>;
	/// Erase data from the storage device.
	/// Guaranteed to be called only with bufs of length a multiple of BLOCK_SIZE.
	fn erase(&mut self, off: usize, len: usize) -> Result<usize>;
	// /// Synchronize writes to the storage device.
	// fn sync(&mut self) -> Result<usize>;
	}

[robin-nitrokey]

I think the tracking issue for this would be: rust-lang/rust#76560

[daringer]

ok tested on nrf extensively and not on lpc55, but all adaptations needed are available as PRs:

• nrf / ext-flash: adapt to new lfs2 lookahead calculation Nitrokey/nitrokey-3-firmware#168 ◀️ nrf adaptations & patches upgraded (also for lpc)
• adapt to trussed-dev/littlefs2#24; rename to LOOKAHEAD_SIZE Nitrokey/lpc55-hal#1 ◀️ lpc adaptations
• virt: adapt to trussed-dev/littlefs2#24 Nitrokey/trussed#12 ◀️ trussed "virt" adaptations

nk3xn & nk3am.bl compile and nk3am.bl has seen ~200 RK operations, ~15 resets, ~50 pin-sets/changes w/o any issues.

Soooo, lgtm 👯

[robin-nitrokey]

Nice! Thanks @daringer! I’ve tested it briefly on lpc55 with the NK3CN (provisioning FIDO2, setting a PIN, registration and authentication with a resident key) and everything worked. For me this PR is ready to merge – any objections?

[daringer]

For me this PR is ready to merge – any objections?

nope, let's go

[nickray]

Before we do so, please remove rust-toolchain.toml instead of bumping it.
I have a fix but couldn't push on top of this PR.

[sosthene-nitrokey]

Regarding the question on whether this is a breaking change or not, it doesn't really matter as 5d31dc5 is clearly a breaking change, so the next release will need to be a major semver anyway.

[szszszsz]

Suggestion: test all applications next time, not only fido-auth.