Add data buffer validation to impl_like() for portable/lean builds #17195
+41
−2
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/17195
Note: Links to docs will display an error until the docs builds have been completed. ❌ 132 New FailuresAs of commit 76e483b with merge base 2ace1cc ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
This PR needs a
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability (TOB-EXECUTORCH-9) by adding data buffer validation to the impl_like() function in bundled_program.cpp, which is used in portable/lean builds. This brings parity with the validation that was added to tensor_like() (used in ATen builds) in PR #17163.
Changes:
- Added buffer size validation to
impl_like()to prevent heap over-read attacks - Added dimension count validation and negative size checks
- Imported
scalar_type_util.hfor theelementSize()function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
95
to
109
| size_t numel = 1; | ||
| for (ssize_t i = 0; i < dim; i++) { | ||
| ET_CHECK(sizes[i] >= 0); | ||
| numel *= static_cast<size_t>(sizes[i]); | ||
| } |
There was a problem hiding this comment.
The numel calculation lacks overflow protection. Multiplying sizes without checking for overflow could result in integer overflow, causing expected_bytes to be much smaller than the actual required size. This would defeat the security validation this code is meant to provide.
Consider using c10::mul_overflows (from c10/util/safe_numerics.h) similar to the pattern in runtime/executor/method_meta.cpp lines 62-70. This should include overflow checks for both the numel calculation and the final multiplication by element size.
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static_cast<int>(sizes[i])); | ||
| numel *= static_cast<size_t>(sizes[i]); | ||
| } | ||
| size_t expected_bytes = numel * executorch::runtime::elementSize(scalar_type); |
There was a problem hiding this comment.
The multiplication of numel by elementSize can also overflow. After fixing the overflow checks for numel calculation, you should also add an overflow check for the final multiplication at this line using c10::mul_overflows, similar to the pattern in runtime/executor/method_meta.cpp:76-82.
Comment on lines
80
to
81
| ScalarType scalar_type = | ||
| static_cast<ScalarType>(bundled_tensor->scalar_type()); |
There was a problem hiding this comment.
Consider adding validation for the scalar_type before using it in elementSize(). While elementSize() does check for invalid types, an early validation with a more specific error message would be more consistent with the pattern in runtime/executor/tensor_parser_portable.cpp:46-50. This would make debugging easier if a malicious file provides an invalid scalar type.
Comment on lines
95
to
110
| size_t numel = 1; | ||
| for (ssize_t i = 0; i < dim; i++) { | ||
| ET_CHECK_MSG( | ||
| sizes[i] >= 0, | ||
| "Tensor has negative size at dimension %zd: %d", | ||
| static_cast<ssize_t>(i), | ||
| static_cast<int>(sizes[i])); | ||
| numel *= static_cast<size_t>(sizes[i]); | ||
| } | ||
| size_t expected_bytes = numel * executorch::runtime::elementSize(scalar_type); |
There was a problem hiding this comment.
The numel calculation is vulnerable to integer overflow. A malicious file could provide dimensions that cause the multiplication to overflow, resulting in a smaller expected_bytes value than intended. This could bypass the buffer size check and lead to heap buffer overflow during tensor operations.
Consider using c10::mul_overflows to detect overflow, similar to the pattern in runtime/executor/method_meta.cpp:62-72. You'll need to include <c10/util/safe_numerics.h> and check overflow at each multiplication step.
| // Validate dimension count | ||
| ET_CHECK( | ||
| dim <= static_cast<ssize_t>(kMaxDim), | ||
| "Tensor rank too large, Max Dim %zu", |
There was a problem hiding this comment.
The error message should include the actual dimension value that caused the failure, not just the limit. This would be more helpful for debugging. Consider a format like: "Tensor rank too large: got %zd, max allowed %zu" with both dim and kMaxDim values, consistent with similar error messages in runtime/executor/tensor_parser_aten.cpp:65 and runtime/executor/tensor_parser_portable.cpp:69.
Suggested change
| "Tensor rank too large, Max Dim %zu", | |
| "Tensor rank too large: got %zd, max allowed %zu", | |
| dim, |
This addresses the security concern (TOB-EXECUTORCH-9) where the non-ATen code path in impl_like() was missing validation that existed in tensor_like(). Followup fix to suggestions: #17163
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.
This addresses the security concern (TOB-EXECUTORCH-9) where the non-ATen code path in impl_like() was missing validation that existed in tensor_like().
Followup fix to suggestions: #17163