Skip to content

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.

Sign up for GitHub

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

Open
jk-arm wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Guard IPA width shift calculation #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9c431b
Harden val_mem_copy bounds checking
jk-arm Dec 19, 2025
5084cd2
Fix void pointer arithmetic in status helpers
jk-arm Jan 7, 2026
1c67e62
Return typed shared region pointers
jk-arm Jan 7, 2026
c8a4844
Guard IPA width shift calculation
jk-arm Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion inc/val_common_framework.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ void val_handle_reboot_result(uint32_t test_progress);
void val_update_regression_report(uint32_t test_result, regre_report_t *regre_report);
void val_print_regression_report(regre_report_t *regre_report);

void val_mem_copy(char *dest, const char *src, size_t len);
void val_mem_copy(char *dest, size_t dest_size, const char *src, size_t len);

#endif /* VAL_COMMON_FRAMEWORK_H */
6 changes: 3 additions & 3 deletions inc/val_common_status.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ typedef struct {
//MSB is set at runtime based on ipa_width selected
#define VAL_NS_SHARED_REGION_IPA_OFFSET 0x700000

void *val_base_addr_ipa(uint64_t ipa_width);
void *val_get_shared_region_base_pa(void);
void *val_get_shared_region_base(void);
uint8_t *val_base_addr_ipa(uint64_t ipa_width);
uint8_t *val_get_shared_region_base_pa(void);
uint8_t *val_get_shared_region_base(void);
void val_set_status(uint32_t status);
uint32_t val_get_status(void);
uint32_t val_report_status(void);
Expand Down
11 changes: 9 additions & 2 deletions src/val_common_framework.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,19 @@ void val_print_regression_report(regre_report_t *regre_report)
/**
* @brief - Copies 'len' bytes from source to destination buffer
* @param - dest : Destination buffer
* - dest_size : Size of destination buffer in bytes
* - src : Source buffer
* - len : Number of bytes to copy
* @return - void
*/
void val_mem_copy(char *dest, const char *src, size_t len)
void val_mem_copy(char *dest, size_t dest_size, const char *src, size_t len)
{
for (size_t i = 0; i < len; ++i)
if (dest == NULL || src == NULL || dest_size == 0 || len == 0)
return;

/* 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];
Comment on lines +183 to 187
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
/* 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';

Copilot uses AI. Check for mistakes.
}
2 changes: 1 addition & 1 deletion src/val_common_log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ uint32_t val_printf(print_verbosity_t verbosity, const char *msg, ...)

if (len > 0 && msg[len - 1] == '\n')
{
val_mem_copy(formatted_msg, msg, len - 1);
val_mem_copy(formatted_msg, sizeof(formatted_msg), msg, len - 1);
formatted_msg[len - 1] = '\r';
formatted_msg[len] = '\n';
formatted_msg[len + 1] = '\0';
Expand Down
36 changes: 27 additions & 9 deletions src/val_common_status.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*
*/

#include <limits.h>

#include "val_common_status.h"
#include "val_common_log.h"

Expand All @@ -15,28 +17,42 @@ static uint64_t width;
* @param ipa_width - Realm IPA width
* @return IPA address of the shared region
**/
void *val_base_addr_ipa(uint64_t ipa_width)
uint8_t *val_base_addr_ipa(uint64_t ipa_width)
{
const uint64_t ptr_bit_width = (uint64_t)(sizeof(uintptr_t) * CHAR_BIT);
const uint64_t max_supported_width =
(ptr_bit_width < 64ull) ? ptr_bit_width : 64ull;

if ((ipa_width == 0ull) || (ipa_width > max_supported_width)) {
val_printf(ERROR,
"Invalid IPA width (%llu). Using shared region base PA.\n",
(unsigned long long)ipa_width);
width = 0;
Copy link

Copilot AI Jan 7, 2026

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'.

Copilot uses AI. Check for mistakes.
return val_get_shared_region_base_pa();
}

width = ipa_width;
return ((void *)(uintptr_t)(VAL_NS_SHARED_REGION_IPA_OFFSET | (1ull << (width - 1))));
uintptr_t ipa_addr = (uintptr_t)(VAL_NS_SHARED_REGION_IPA_OFFSET |
(1ull << (width - 1ull)));
return (uint8_t *)ipa_addr;
}

/**
* @brief Returns the base address of the shared region
* @param Void
* @return Physical address of the shared region
**/
void *val_get_shared_region_base_pa(void)
uint8_t *val_get_shared_region_base_pa(void)
{
return ((void *)(PLATFORM_SHARED_REGION_BASE));
return (uint8_t *)(uintptr_t)(PLATFORM_SHARED_REGION_BASE);
}

/**
* @brief Returns the base address of the shared region
* @param Void
* @return Base address of the shared region
**/
void *val_get_shared_region_base(void)
uint8_t *val_get_shared_region_base(void)
{
if (width)
return val_base_addr_ipa(width);
Expand All @@ -52,8 +68,9 @@ void *val_get_shared_region_base(void)
void val_set_status(uint32_t status)
{
uint8_t state = ((status >> TEST_STATE_SHIFT) & TEST_STATE_MASK);
val_test_status_buffer_ts *curr_test_status = (val_get_shared_region_base()
+ TEST_STATUS_OFFSET);
uint8_t *shared_region_base = val_get_shared_region_base();
val_test_status_buffer_ts *curr_test_status =
(val_test_status_buffer_ts *)(shared_region_base + TEST_STATUS_OFFSET);

curr_test_status->state = state;
curr_test_status->status_code = (status & TEST_STATUS_CODE_MASK);
Expand All @@ -66,8 +83,9 @@ void val_set_status(uint32_t status)
**/
uint32_t val_get_status(void)
{
val_test_status_buffer_ts *curr_test_status = (val_get_shared_region_base()
+ TEST_STATUS_OFFSET);
uint8_t *shared_region_base = val_get_shared_region_base();
val_test_status_buffer_ts *curr_test_status =
(val_test_status_buffer_ts *)(shared_region_base + TEST_STATUS_OFFSET);
return (uint32_t)(((curr_test_status->state) << TEST_STATE_SHIFT) |
(curr_test_status->status_code));
}
Expand Down
Binary file added val_common_status_fix.diff
View file
Open in desktop
Binary file not shown.