core: Gracefully handle allocations that overflow size_t. #18020
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
809c468 to
5cfd078
Compare
|
Code size report: |
Frequently, allocations are sized based on a computation like `base_size + count * entry_size`. If an integer overflow occurs in computation and is not checked for, this can lead to success with a smaller-than-intended allocation, followed by out-of-bounds access to the buffer. Add a utility function `mp_compute_size_overflow` to compute `base_size + count * entry_size` and use it in previously identified contexts: * the m_new and m_renew family of macros, which know the type and count of the objects they allocate * list, str, and tuple BINARY_OP_MULTIPLY, which create repetitions of a sequence. In the latter case, note that overflow can occur in the a first step (when calculating the new *count of elements*) or in a second step (when calculating the *number of bytes*). `mp_compute_size_overflow` doesn't directly raise an exception; instead, it returns SIZE_MAX, assuming no allocation of exactly SIZE_MAX can ever succeed. The existing code paths either return NULL or raise an exception, just as before. At the current time none of the paths with m_malloc_with_finaliser (mp_obj_malloc_var_with_finaliser) seem to have an uncontrolled size parameter. The exception path will show an incorrect attempted allocation size e.g., 4294967295, in this case. Signed-off-by: Jeff Epler <jepler@gmail.com>
5cfd078 to
2444b9c
Compare
Signed-off-by: Jeff Epler <jepler@gmail.com>
This probably only saves code when MICROPY_MEM_STATS is enabled. Signed-off-by: Jeff Epler <jepler@gmail.com>
These are still prone to overflow. Signed-off-by: Jeff Epler <jepler@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18020 +/- ##
=======================================
Coverage 98.38% 98.39%
=======================================
Files 171 171
Lines 22296 22306 +10
=======================================
+ Hits 21937 21947 +10
Misses 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3cc00a2 to
b7ffce8
Compare
Signed-off-by: Jeff Epler <jepler@gmail.com>
b7ffce8 to
d518e7f
Compare
Signed-off-by: Jeff Epler <jepler@gmail.com>
|
natmod doesn't actually check for overflows, but it does build again. |
|
This is an interesting, if 10+ year old, review of programming errors in memory allocators, focusing on overflows: https://kqueue.org/blog/2012/03/05/memory-allocator-security-revisited/ While several allocators investigated there had problems with allocations of SIZE_MAX returning non-NULL, the usual micropython allocator works properly in this case. |
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
Frequently, allocations are sized based on a computation like
base_size + count * entry_sizeor justcount * entry_size.If an integer overflow occurs in computation and is not checked for, this can lead to success with a smaller-than-intended allocation, followed by out-of-bounds access to the buffer. This is the underlying cause of #17925.
Add a utility function
mp_compute_size_overflowto computebase_size + count * entry_sizeand use it in previously identified contexts:In the latter case, note that overflow can occur in the a first step (when calculating the new count of elements) or in a second step (when calculating the number of bytes).
mp_compute_size_overflowdoesn't directly raise an exception; instead, it returns SIZE_MAX, assuming no allocation of exactly SIZE_MAX can ever succeed. The existing code paths either return NULL or raise an exception, just as before.The exception path will show an incorrect attempted allocation size e.g., 4294967295, in this case.
Closes: #17925
Testing
I'm relying on CI testing to find any regressions introduced by this change.
Trade-offs and Alternatives
The main alternative is to do nothing and accept that these constructs can cause crashes or wrong behavior. I have not identified any specific non-crashing behavior, but overflows of this type have been recognized as having security impacts as in e.g.,https://seclists.org/bugtraq/2002/Aug/91