[scudo] Fix stack depot validation. #87024
Conversation
In the StackDepot::isValid function, there is work to validate the TabMask variable. Unfortunately, if TabMask is set to the maximum allowed value, TabSize = TabMask + 1 becomes zero and validation passes. Disallow that case to prevent invalid reads into the Tab structure.
llvmbot
added
compiler-rt
compiler-rt:scudo
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Christopher Ferris (cferris1000) ChangesIn the StackDepot::isValid function, there is work to validate the TabMask variable. Unfortunately, if TabMask is set to the maximum allowed value, TabSize = TabMask + 1 becomes zero and validation passes. Disallow that case to prevent invalid reads into the Tab structure. Full diff: https://github.com/llvm/llvm-project/pull/87024.diff 1 Files Affected:
diff --git a/compiler-rt/lib/scudo/standalone/stack_depot.h b/compiler-rt/lib/scudo/standalone/stack_depot.h
index cf3cabf7085b60..98cd9707a64613 100644
--- a/compiler-rt/lib/scudo/standalone/stack_depot.h
+++ b/compiler-rt/lib/scudo/standalone/stack_depot.h
@@ -112,7 +112,7 @@ class alignas(atomic_u64) StackDepot {
if (TabMask == 0)
return false;
uptr TabSize = TabMask + 1;
- if (!isPowerOfTwo(TabSize))
+ if (TabSize == 0 || !isPowerOfTwo(TabSize))
return false;
uptr TabBytes = sizeof(atomic_u32) * TabSize;
|
| if (TabSize == 0 || !isPowerOfTwo(TabSize)) | ||
| return false; |
There was a problem hiding this comment.
I'm wondering if we should fix isPowerOfTwo instead? Zero is not a power of 2.
There was a problem hiding this comment.
It might be worth doing that, but I do worry that somewhere, something depends on the behavior.
I'll file a bug because I think that's a bigger change and requires some extra verification.
There was a problem hiding this comment.
I did a quick review and it seems to be fine (there's no additional zero check will be introduced). And yes, it's fine to track it later
In the StackDepot::isValid function, there is work to validate the TabMask variable. Unfortunately, if TabMask is set to the maximum allowed value, TabSize = TabMask + 1 becomes zero and validation passes.
Disallow that case to prevent invalid reads into the Tab structure.