Conversation
b1c0e3b to
a2d630f
Compare
There was a problem hiding this comment.
Pull request overview
This PR enhances the Vorago VA416x0 platform support by improving script portability for macOS, adding wolfCrypt test/benchmark capabilities to the test application, and implementing partition size validation.
Changes:
- Fixed JLinkExe detection for macOS compatibility in the build script
- Added wolfCrypt test and benchmark support to test-app with necessary infrastructure
- Implemented partition size checking to prevent oversized images
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/test.mk | Updated ML_DSA size limit for test |
| tools/scripts/va416x0/build_test.sh | Added macOS JLinkExe detection and partition size validation |
| tools/keytools/sign.py | Added partition size overflow checking |
| tools/keytools/sign.c | Added partition size overflow checking |
| test-app/wolfcrypt_support.c | New file providing time and RNG functions for wolfCrypt |
| test-app/syscalls.c | New file providing newlib syscall stubs for embedded systems |
| test-app/app_va416x0.c | Added wolfCrypt test/benchmark integration |
| test-app/Makefile | Added build support for wolfCrypt test/benchmark |
| test-app/ARM-va416x0.ld | Increased heap and stack sizes |
| src/update_flash.c | Fixed MAX_UPDATE_SIZE calculation to include header size |
| include/user_settings.h | Added wolfCrypt test/benchmark configuration options |
| config/examples/vorago_va416x0.config | Updated partition layout and optimization settings |
| Makefile | Added WOLFBOOT_PARTITION_SIZE to sign tool environment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| JLINK_PATH=$(find /Applications/SEGGER -name "JLinkExe" 2>/dev/null | head -n1) | ||
| if [ -n "$JLINK_PATH" ] && [ -x "$JLINK_PATH" ]; then | ||
| JLINK="$JLINK_PATH" | ||
| else | ||
| echo "Error: JLinkExe not found. Please install SEGGER J-Link software." |
There was a problem hiding this comment.
The error message on line 28 doesn't account for the case where /Applications/SEGGER doesn't exist. The find command will fail silently but the error message will still say 'JLinkExe not found' which could be confusing. Consider adding a check for the directory's existence first or improving the error message to indicate both possible failure modes.
| JLINK_PATH=$(find /Applications/SEGGER -name "JLinkExe" 2>/dev/null | head -n1) | |
| if [ -n "$JLINK_PATH" ] && [ -x "$JLINK_PATH" ]; then | |
| JLINK="$JLINK_PATH" | |
| else | |
| echo "Error: JLinkExe not found. Please install SEGGER J-Link software." | |
| if [ -d /Applications/SEGGER ]; then | |
| JLINK_PATH=$(find /Applications/SEGGER -name "JLinkExe" 2>/dev/null | head -n1) | |
| if [ -n "$JLINK_PATH" ] && [ -x "$JLINK_PATH" ]; then | |
| JLINK="$JLINK_PATH" | |
| else | |
| echo "Error: JLinkExe not found in PATH or under /Applications/SEGGER. Please install SEGGER J-Link software." | |
| exit 1 | |
| fi | |
| else | |
| echo "Error: JLinkExe not found in PATH and /Applications/SEGGER does not exist. Please install SEGGER J-Link software." |
| prev_heap_end = heap_end; | ||
|
|
||
| /* Calculate heap limit */ | ||
| heap_limit = &_end + _Min_Heap_Size; |
There was a problem hiding this comment.
Pointer arithmetic error: _Min_Heap_Size is an extern char (treated as pointer), not an integer value. This should cast _Min_Heap_Size to an integer type (e.g., (int)&_Min_Heap_Size) to correctly calculate the heap limit.
|
|
||
| void _exit(int status) | ||
| { | ||
| while(1); |
There was a problem hiding this comment.
Missing space between 'while' and '(1)'. Should be 'while (1);' for consistency with standard C formatting conventions.
| while(1); | |
| while (1); |
| #endif | ||
|
|
||
| /* Reserve space for two sectors in case of NVM_FLASH_WRITEONCE, for redundancy */ | ||
| /* Max firmware size: partition must hold header + fw + trailer sector(s) */ |
There was a problem hiding this comment.
The comment on line 647 is now misleading. It says 'partition must hold header + fw + trailer sector(s)' but the calculation subtracts both IMAGE_HEADER_SIZE and WOLFBOOT_SECTOR_SIZE from WOLFBOOT_PARTITION_SIZE, meaning MAX_UPDATE_SIZE represents only the firmware size, not including the header. Update the comment to clarify that MAX_UPDATE_SIZE is the maximum firmware size (excluding header).
| /* Max firmware size: partition must hold header + fw + trailer sector(s) */ | |
| /* Max firmware size (excluding header); partition must also hold header + | |
| * trailer sector(s) */ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
tools/keytools/sign.py:97
- Inside
make_header(), assignments toWOLFBOOT_PARTITION_SIZE/WOLFBOOT_SECTOR_SIZEcreate locals unless declaredglobal, so the later size check will still see the module-level values (currently 0) and never run. Declare these asglobalin the function (or refactor to pass them through) so the partition-size guard actually takes effect.
def make_header(image_file, fw_version, extra_fields=[]):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if command -v JLinkExe &> /dev/null; then | ||
| JLINK="JLinkExe" |
There was a problem hiding this comment.
&> /dev/null is not POSIX-sh compatible (it’s a bash-ism). If this script is run under /bin/sh (common on macOS/Linux), this can fail. Prefer >/dev/null 2>&1, or explicitly set the shebang to bash if bash is required.
| if "WOLFBOOT_PARTITION_SIZE" in l and "ADDRESS" not in l: | ||
| val=l.split('=')[1].rstrip('\n') | ||
| WOLFBOOT_PARTITION_SIZE = int(val,0) | ||
| if "WOLFBOOT_SECTOR_SIZE" in l: | ||
| val=l.split('=')[1].rstrip('\n') | ||
| WOLFBOOT_SECTOR_SIZE = int(val,0) |
There was a problem hiding this comment.
Inside make_header(), assignments to WOLFBOOT_PARTITION_SIZE / WOLFBOOT_SECTOR_SIZE create locals unless declared global, so the later size check will still see the module-level values (currently 0) and never run. Declare these as global in the function (or refactor to pass them through) so the partition-size guard actually takes effect.
| extern char _end; /* Defined by linker */ | ||
| extern char _Min_Heap_Size; /* Defined by linker (if available) */ |
There was a problem hiding this comment.
_Min_Heap_Size is a linker symbol value, but it’s being used as if it were a char variable (&_end + _Min_Heap_Size). This will either fail to compile (invalid operands) or read a byte from an address that is not intended to be dereferenced. Use the symbol’s address/value correctly (e.g., cast (uintptr_t)&_Min_Heap_Size as the size), or define/export an explicit heap limit symbol in the linker script and compare against that.
| prev_heap_end = heap_end; | ||
|
|
||
| /* Calculate heap limit */ | ||
| heap_limit = &_end + _Min_Heap_Size; |
There was a problem hiding this comment.
_Min_Heap_Size is a linker symbol value, but it’s being used as if it were a char variable (&_end + _Min_Heap_Size). This will either fail to compile (invalid operands) or read a byte from an address that is not intended to be dereferenced. Use the symbol’s address/value correctly (e.g., cast (uintptr_t)&_Min_Heap_Size as the size), or define/export an explicit heap limit symbol in the linker script and compare against that.
| APP_OBJS+=syscalls.o | ||
|
|
There was a problem hiding this comment.
When WOLFCRYPT_SUPPORT=1, you compile/link your own syscall stubs (syscalls.o) while also linking with --specs=nosys.specs (which typically pulls in libnosys syscall stubs). This commonly causes multiple-definition link errors for _write, _sbrk, etc. Consider making --specs=nosys.specs conditional (disable it when syscalls.o is linked), or drop syscalls.o and rely on nosys (with any required overrides).
| APP_OBJS+=syscalls.o |
| APP_OBJS+=../src/keystore.o | ||
| endif | ||
| # Reduce size: newlib-nano and section GC | ||
| LDFLAGS+=--specs=nano.specs --specs=nosys.specs |
There was a problem hiding this comment.
When WOLFCRYPT_SUPPORT=1, you compile/link your own syscall stubs (syscalls.o) while also linking with --specs=nosys.specs (which typically pulls in libnosys syscall stubs). This commonly causes multiple-definition link errors for _write, _sbrk, etc. Consider making --specs=nosys.specs conditional (disable it when syscalls.o is linked), or drop syscalls.o and rely on nosys (with any required overrides).
| LDFLAGS+=--specs=nano.specs --specs=nosys.specs | |
| LDFLAGS+=--specs=nano.specs | |
| ifneq ($(WOLFCRYPT_SUPPORT),1) | |
| # Only pull in nosys stubs when not providing our own syscalls | |
| LDFLAGS+=--specs=nosys.specs | |
| endif |
| uint32_t partition_sz = (uint32_t)strtol(env_psize, NULL, 0); | ||
| uint32_t sector_sz = env_ssize ? | ||
| (uint32_t)strtol(env_ssize, NULL, 0) : 0; | ||
| uint32_t total_img_sz = CMD.header_sz + image_sz; | ||
| /* Only subtract sector for trailer when sector < partition. | ||
| * When sector >= partition (e.g. update_ram targets), the | ||
| * entire partition is available for the image. */ | ||
| uint32_t max_img_sz = (sector_sz < partition_sz) ? | ||
| (partition_sz - sector_sz) : partition_sz; | ||
| if (total_img_sz > max_img_sz) { | ||
| printf("Error: Image size %u (header %u + firmware %u) " | ||
| "exceeds max %u (partition %u - sector %u)\n", | ||
| total_img_sz, CMD.header_sz, image_sz, | ||
| max_img_sz, partition_sz, sector_sz); | ||
| goto failure; | ||
| } |
There was a problem hiding this comment.
The strtol(..., NULL, 0) conversions ignore parse errors and allow negative values, which then get cast to uint32_t and can produce very large sizes. This can make the size check behave incorrectly. Use an endptr and validate that the full string was parsed, and reject non-positive / out-of-range values (fail early with a clear error).
| uint32_t partition_sz = (uint32_t)strtol(env_psize, NULL, 0); | |
| uint32_t sector_sz = env_ssize ? | |
| (uint32_t)strtol(env_ssize, NULL, 0) : 0; | |
| uint32_t total_img_sz = CMD.header_sz + image_sz; | |
| /* Only subtract sector for trailer when sector < partition. | |
| * When sector >= partition (e.g. update_ram targets), the | |
| * entire partition is available for the image. */ | |
| uint32_t max_img_sz = (sector_sz < partition_sz) ? | |
| (partition_sz - sector_sz) : partition_sz; | |
| if (total_img_sz > max_img_sz) { | |
| printf("Error: Image size %u (header %u + firmware %u) " | |
| "exceeds max %u (partition %u - sector %u)\n", | |
| total_img_sz, CMD.header_sz, image_sz, | |
| max_img_sz, partition_sz, sector_sz); | |
| goto failure; | |
| } | |
| char *endptr; | |
| unsigned long tmp_ul; | |
| uint32_t partition_sz; | |
| uint32_t sector_sz = 0; | |
| errno = 0; | |
| tmp_ul = strtoul(env_psize, &endptr, 0); | |
| if (endptr == env_psize || *endptr != '\0' || errno == ERANGE || | |
| tmp_ul == 0 || tmp_ul > UINT32_MAX) { | |
| printf("Error: Invalid WOLFBOOT_PARTITION_SIZE value '%s'\n", | |
| env_psize); | |
| goto failure; | |
| } | |
| partition_sz = (uint32_t)tmp_ul; | |
| if (env_ssize) { | |
| errno = 0; | |
| tmp_ul = strtoul(env_ssize, &endptr, 0); | |
| if (endptr == env_ssize || *endptr != '\0' || errno == ERANGE || | |
| tmp_ul == 0 || tmp_ul > UINT32_MAX) { | |
| printf("Error: Invalid WOLFBOOT_SECTOR_SIZE value '%s'\n", | |
| env_ssize); | |
| goto failure; | |
| } | |
| sector_sz = (uint32_t)tmp_ul; | |
| } | |
| { | |
| uint32_t total_img_sz = CMD.header_sz + image_sz; | |
| /* Only subtract sector for trailer when sector < partition. | |
| * When sector >= partition (e.g. update_ram targets), the | |
| * entire partition is available for the image. */ | |
| uint32_t max_img_sz = (sector_sz < partition_sz) ? | |
| (partition_sz - sector_sz) : partition_sz; | |
| if (total_img_sz > max_img_sz) { | |
| printf("Error: Image size %u (header %u + firmware %u) " | |
| "exceeds max %u (partition %u - sector %u)\n", | |
| total_img_sz, CMD.header_sz, image_sz, | |
| max_img_sz, partition_sz, sector_sz); | |
| goto failure; | |
| } | |
| } |
| #if defined(WOLFCRYPT_TEST) || defined(WOLFCRYPT_BENCHMARK) | ||
| /* Use custom RNG for tests (saves ~7KB vs HASHDRBG) */ | ||
| #define WC_NO_HASHDRBG | ||
| #define CUSTOM_RAND_GENERATE_SEED my_rng_seed_gen | ||
| #define CUSTOM_RAND_GENERATE_BLOCK my_rng_seed_gen | ||
| extern int my_rng_seed_gen(unsigned char* output, unsigned int sz); | ||
| #else |
There was a problem hiding this comment.
This wires a deterministic, non-cryptographic RNG into wolfCrypt whenever WOLFCRYPT_TEST/WOLFCRYPT_BENCHMARK is enabled. That’s fine for tests, but it’s a sharp edge if these flags are accidentally enabled outside test builds. Consider adding a stronger compile-time safeguard (e.g., require an explicit ALLOW_INSECURE_TEST_RNG macro, or emit a preprocessor error unless a dedicated test configuration is selected) to prevent shipping builds with this RNG.
Fix for tools/scripts/va416x0/build_test.sh portability (MacOS)
Added option to support wolfCrypt test/benchmark in test-app
Add some checking if partition size is too large