Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix filesystem corruption #19

Closed
wants to merge 1 commit into from
Closed

Fix filesystem corruption #19

wants to merge 1 commit into from

Conversation

arturkow2000
Copy link

@arturkow2000 arturkow2000 commented Jun 30, 2022 •
edited
Loading

LittleFS bindings reported too big lookahead cache size which resulted
in corrupting adjacent structures (read and prog buffer), which in turn
resulted in filesystem corruption either by reallocating an already
allocated causing cycle or by corrupting data before writing it to disk.

Fixes: https://github.com/nickray/littlefs2/issues/16

@arturkow2000 arturkow2000 marked this pull request as draft June 30, 2022 08:46
@arturkow2000
Fix filesystem corruption
7514184 
LittleFS bindings reported too big lookahead cache size which resulted
in corrupting adjacent structures (read and prog buffer, other data),
which in turn resulted in filesystem corruption either by reallocating
an already allocated causing cycle or by corrupting data before writing
it to disk.
@arturkow2000 arturkow2000 marked this pull request as ready for review June 30, 2022 13:27
@arturkow2000 arturkow2000 mentioned this pull request Jun 30, 2022
Closed
@lexxvir
Copy link

lexxvir commented Jul 6, 2022

Hi @arturkow2000 !

Thank you for the fix! It fixes the FS corruption I encountered too.

Hi @nickray,

Please, merge the fix, thank you!

@svenstaro
Copy link

@nickray are you still maintaining this? Not to hassle you but this seems like a fairly critical issue. Perhaps it might be helpful to add more maintainers if you don't have much time at the moment?

@robin-nitrokey robin-nitrokey mentioned this pull request Jan 19, 2023
Open
@robin-nitrokey
Copy link
Member

Note that this change also requires a change to the ram_storage and const_ram_storage macros: As the lookahead size has to be a multiple of 8, LOOKAHEADWORDS_SIZE must be at least 2.

https://github.com/littlefs-project/littlefs/blob/6a53d76e90af33f0656333c1db09bd337fa75d23/lfs.h#L226-L230
Nitrokey#1

I’m not sure where the factor 4 comes from here. Wouldn’t it make more sense to use 8 directly?

@robin-nitrokey
Copy link
Member

I’m not sure where the factor 4 comes from here. Wouldn’t it make more sense to use 8 directly?

Ah, figured it out: The lookahead buffer uses u32, not u64. So the factor is 32 / 8 = 4.

In the future, we should change that so that users cannot specify invalid lookahead sizes.

@nickray
Copy link
Member

nickray commented Jan 25, 2023 •
edited
Loading

@nickray are you still maintaining this? Not to hassle you but this seems like a fairly critical issue. Perhaps it might be helpful to add more maintainers if you don't have much time at the moment?

@svenstaro These littlefs bindings are now moved under the Trussed org, where Nitrokey can co-maintain.

robin-nitrokey added a commit to Nitrokey/littlefs2 that referenced this pull request Jan 26, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
1a0b649 
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:
	trussed-dev#19
	#1

Fixes: trussed-dev#16
@robin-nitrokey robin-nitrokey mentioned this pull request Jan 26, 2023
Merged
robin-nitrokey added a commit to Nitrokey/littlefs2 that referenced this pull request Jan 27, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
694c6a8 
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:
	trussed-dev#19
	#1

Fixes: trussed-dev#16
robin-nitrokey added a commit to Nitrokey/littlefs2 that referenced this pull request Feb 3, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
cc79291 
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:
	trussed-dev#19
	#1

Fixes: trussed-dev#16
robin-nitrokey added a commit to Nitrokey/littlefs2 that referenced this pull request Feb 7, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
2c7bbad 
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:
	trussed-dev#19
	#1

Fixes: trussed-dev#16
robin-nitrokey added a commit to Nitrokey/littlefs2 that referenced this pull request Feb 7, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
82ac3eb 
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:
	trussed-dev#19
	#1

Fixes: trussed-dev#16
robin-nitrokey added a commit that referenced this pull request Feb 7, 2023
@robin-nitrokey
Fix lookahead buffer size reported to littlefs2-sys
aafa02e 
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
@robin-nitrokey
Copy link
Member

Included in #24.

@macpijan macpijan deleted the fs-corruption-fix branch May 22, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arturkow2000 @lexxvir @svenstaro @robin-nitrokey @nickray