-
Notifications
You must be signed in to change notification settings - Fork 2
Guard IPA width shift calculation #43
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds input validation for IPA width calculations to prevent undefined behavior from invalid shift operations. The key changes guard against zero or oversized width values that would cause out-of-range bit shifts, and include a safety improvement to the memory copy function.
- Adds validation logic to detect invalid IPA widths before performing bit shift operations
- Changes return types from
void *touint8_t *for shared region base address functions to support proper pointer arithmetic - Enhances
val_mem_copywith destination buffer size parameter and bounds checking
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/val_common_status.c | Implements IPA width validation with fallback to platform base, adds <limits.h> for portable bit width checks, refactors pointer arithmetic to use explicit uint8_t * |
| inc/val_common_status.h | Updates function signatures to return uint8_t * instead of void * |
| src/val_common_framework.c | Adds destination buffer size parameter and bounds checking to val_mem_copy |
| inc/val_common_framework.h | Updates val_mem_copy signature with dest_size parameter |
| src/val_common_log.c | Updates val_mem_copy call to include destination buffer size |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val_printf(ERROR, | ||
| "Invalid IPA width (%llu). Using shared region base PA.\n", | ||
| (unsigned long long)ipa_width); | ||
| width = 0; |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the global width variable to 0 in the error path creates inconsistent state. If val_get_shared_region_base() is later called, it will return the platform shared region base (line 60), but the global width being 0 signals that no IPA width has been successfully set. Consider documenting this behavior or using a sentinel value to distinguish between 'not yet initialized' and 'validation failed'.
| /* Clamp requested length so we never write beyond the destination buffer */ | ||
| size_t bytes_to_copy = len < dest_size ? len : dest_size; | ||
|
|
||
| for (size_t i = 0; i < bytes_to_copy; ++i) | ||
| dest[i] = src[i]; |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds check should account for null termination if dest is intended to be a null-terminated string. When len == dest_size, copying all bytes leaves no room for a null terminator, potentially causing buffer overruns if the destination is later treated as a string. Consider using len < dest_size ? len : dest_size - 1 and explicitly null-terminating the destination, or document that this function does not null-terminate.
| /* Clamp requested length so we never write beyond the destination buffer */ | |
| size_t bytes_to_copy = len < dest_size ? len : dest_size; | |
| for (size_t i = 0; i < bytes_to_copy; ++i) | |
| dest[i] = src[i]; | |
| /* Clamp requested length so we never write beyond the destination buffer | |
| * and leave room for a null terminator. | |
| */ | |
| size_t max_copy = dest_size - 1; | |
| size_t bytes_to_copy = (len < max_copy) ? len : max_copy; | |
| for (size_t i = 0; i < bytes_to_copy; ++i) | |
| dest[i] = src[i]; | |
| /* Ensure destination is null-terminated */ | |
| dest[bytes_to_copy] = '\0'; |
Summary
Testing