extmod/vfs_rom: Add bounds checking for all filesystem accesses. #16810
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16810 +/- ##
=======================================
Coverage 98.53% 98.53%
=======================================
Files 169 169
Lines 21822 21852 +30
=======================================
+ Hits 21502 21532 +30
Misses 320 320 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
b299a42 to
a06c9b0
Compare
|
I needed to increase the MicroPython heap for the qemu Sabrelite board, because the new ROMFS test is quite large when compiled using the ARM (non-Thumb) native emitter. |
projectgus
approved these changes
Feb 25, 2025
Signed-off-by: Damien George <damien@micropython.org>
Testing with ROMFS shows that it is relatively easy to end up with a corrupt filesystem on the device -- eg due to the ROMFS deploy process stopping half way through -- which could lead to hard crashes. Notably, there can be boot loops trying to mount a corrupt filesystem, crashes when importing modules like `os` that first scan the filesystem for `os.py`, and crashing when deploying a new ROMFS in certain cases because the old one is removed while still mounted. The main problem is that `mp_decode_uint()` has an loop that keeps going as long as it reads 0xff byte values, which can happen in the case of erased and unwritten flash. This commit adds full bounds checking in the new `mp_decode_uint_checked()` function, and that makes all ROMFS filesystem accesses robust. Signed-off-by: Damien George <damien@micropython.org>
7429b8f to
14ba32b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
While testing ROMFS as part of #8381 I found that it was relatively easy to end up with a corrupt filesystem on the device (eg due to the ROMFS deploy process stopping half way through), which could lead to hard crashes. Notably: boot loops trying to mount a corrupt filesystem, crashes when importing modules like
osthat first scan the filesystem foros.py, and crashing when deploying a new ROMFS in certain cases because the old one is removed while still mounted.@robert-hh also encountered similar issues when testing #8381.
This PR adds full bounds checking for all ROMFS filesystem accesses.
Testing
The new code should be fully covered by the new tests that are added here, and they run in CI.
Trade-offs and Alternatives
I was hoping such bounds checking would not be necessary, because it increases the size and complexity of the ROMFS driver. But after playing around with ROMFS in a project, it's definitely necessary to have full bounds checking.