Core firmware split #4188
Core firmware split #4188
Conversation
[no changelog]
[no changelog]
[no changelog]
cepetr
requested review from
andrewkozlik,
matejcik and
prusnak
as code owners
cepetr
force-pushed
the
cepetr/core-split
branch
from
58e347f
to
ebcda42
Compare
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
[no changelog]
cepetr
force-pushed
the
cepetr/core-split
branch
from
26d1025
to
3c561cf
Compare
core/embed/trezorhal/stm32u5/xdisplay/stm32u5a9j-dk/display_ltdc_dsi.c
Outdated
Show resolved
Hide resolved
| typedef enum { | ||
| SECURE_AES_KEY_DHUK, |
There was a problem hiding this comment.
@TychoVrahe I would prefer to remove all enum variants ending with _NP and _NN, as they are unused and unlikely to be needed in the future.
| // reset the key loaded in SAES | ||
| MODIFY_REG(SAES->CR, AES_CR_KEYSEL, CRYP_KEYSEL_NORMAL); | ||
|
|
||
| register uint32_t r0 __asm__("r0") = sectrue; |
There was a problem hiding this comment.
@TychoVrahe You can call syscall_return_from_callback() here. The function name may not be ideal, but it performs the same task.
There was a problem hiding this comment.
used in 30248cb , had to compile the function for kernel too.
|
|
||
| #define KERNEL_UNPRIVILEGED_SIZE 32 |
There was a problem hiding this comment.
@TychoVrahe KERNEL_UNPRIVILEGED_SIZE is a bit of an odd name. How about using SHA256_BLOCK_SIZE instead? We might already have a constant like that defined somewhere.
There was a problem hiding this comment.
renamed to a bit different name than suggested, as this, in principle, has nothing to do with SHA256,
see 30248cb , let me know if you agree
| *m = 0; | ||
| } | ||
|
|
||
| for (int i = 0; i < KERNEL_UNPRIVILEGED_SIZE; i++) { |
There was a problem hiding this comment.
@TychoVrahe It might be more semantically appropriate to use size variable instead of the constant here. The same applies to the code below.
| return secfalse; | ||
| } | ||
|
|
||
| NVIC_SetPriority(SVCall_IRQn, IRQ_PRI_HIGHEST); |
There was a problem hiding this comment.
@TychoVrahe I would be a bit safer to use NVIC_GetPriority and store the previous value.
| __HAL_RCC_SAES_RELEASE_RESET(); | ||
| __HAL_RCC_SAES_CLK_ENABLE(); | ||
|
|
||
| for (int i = 0; i < KERNEL_UNPRIVILEGED_SIZE; i++) { |
There was a problem hiding this comment.
@TychoVrahe Why not to use memset & memcpy?
| @@ -136,8 +290,28 @@ secbool secure_aes_ecb_encrypt_hw(const uint8_t* input, size_t size, | |||
| return sectrue; | |||
| } | |||
|
|
|||
| secbool secure_aes_init(void) { | |||
There was a problem hiding this comment.
@TychoVrahe What do you think about relocating secure_aes_init() either before or after the encrypt and decrypt routines? It might be a bit confusing when placed between them.
There was a problem hiding this comment.
moved to the end 30248cb
|
|
||
| NVIC_SetPriority(SVCall_IRQn, IRQ_PRI_HIGHEST); | ||
| uint32_t basepri = __get_BASEPRI(); | ||
| __set_BASEPRI(1); |
There was a problem hiding this comment.
@TychoVrahe This might better convey the intention: _set_BASEPRI(1) -> __set_BASEPRI(IRQ_PRI_HIGHEST + 1). I’m not entirely happy with it, but I don’t have a better suggestion at the moment.
[no changelog]
| FLASH (rx) : ORIGIN = 0x08000000, LENGTH = 48K | ||
| CCMRAM (wal) : ORIGIN = 0x10000000, LENGTH = 64K | ||
| SRAM (wal) : ORIGIN = 0x20000000, LENGTH = 192K | ||
| FLASH (rx) : ORIGIN = MCU_FLASH_ORIGIN, LENGTH = BOARDLOADER_IMAGE_MAXSIZE |
There was a problem hiding this comment.
@TychoVrahe Shouldn't we use BOARDLOADER_START instead of MCU_FLASH_ORIGIN here?
| } | ||
|
|
||
| main_stack_base = ORIGIN(CCMRAM) + LENGTH(CCMRAM); /* 8-byte aligned full descending stack */ | ||
| _sstack = ORIGIN(CCMRAM); |
There was a problem hiding this comment.
@TychoVrahe We could rename _sstack and _estack to _stack_addr and _stack_end, and add an underscore prefix to data_lma, data_vma, bss_start, etc., so that all symbols coming from linker scripts have an underscore prefix. However, I would prefer to do this in a subsequent PR.
| @@ -92,7 +95,7 @@ void drivers_init() { | |||
| dma2d_init(); | |||
| #endif | |||
|
|
|||
| display_init(DISPLAY_RETAIN_CONTENT); | |||
| display_init(DISPLAY_JUMP_BEHAVIOR); | |||
There was a problem hiding this comment.
@TychoVrahe What about renaming this to something that includes the word "handover" (to be consistent with the symbol in the linker script)? Something like DISPLAY_CONTENT_AFTER_HANDOVER... It’s a bit weird, but it might better convey the intent.
There was a problem hiding this comment.
hmm the functions we use to move between stages are still called jump, so there gonna be inconsistencies anyway until we can rename it all. I slightly prefer to keep current name until we do that
core/embed/kernel/main.c
Outdated
| @@ -166,17 +169,24 @@ void drivers_init() { | |||
| #endif | |||
| } | |||
|
|
|||
| // defined in linker script | |||
| extern uint32_t _codelen; | |||
| extern uint32_t _applet_clear_ram_0_start; | |||
There was a problem hiding this comment.
@TychoVrahe Not sure about this, but I think these symbols are more like _coreapp_clear_ram_xxx then _aplet_clear_ram_xx, because applet is more general.
| target=target_, | ||
| source=file, | ||
| action="$OBJCOPY -I binary -O elf32-littlearm -B arm" | ||
| f" --rename-section .data=.{section}" + " $SOURCE $TARGET", |
There was a problem hiding this comment.
@TychoVrahe What's the purpose of renaming the .data section here?
There was a problem hiding this comment.
The objcopy input target is -I binary, so everything in that file is considered .data, so i need to differentiate it in linker script somehow to ensure proper placement. And also need to ensure that it isn't dropped by linker since it otherwise seems unused.
I could also do something like this in linker script
KEEP(build/firmware/build/kernel/kernel.o(.data));
instead of current
KEEP(*(.kernel));
but if find current solution a bit nicer.
| @@ -6,14 +6,12 @@ | |||
| .type reset_handler, STT_FUNC | |||
There was a problem hiding this comment.
@TychoVrahe We're a bit inconsistent with the startup_xxx file extensions (.s vs .S). I think we should unify them. Maybe .S is more versatile since it is processed by the C preprocessor and can contain #ifdefs, but we don't necessarily need that here.
There was a problem hiding this comment.
renamed to .s.We used to have ifdefs for model one in here, but its not needed, for now.
6f0da0b
| @@ -24,15 +24,15 @@ | |||
|
|
|||
| #define IMAGE_CHUNK_SIZE (128 * 1024) | |||
| #define IMAGE_HASH_BLAKE2S | |||
| #define BOARD_CAPABILITIES_ADDR 0x0800BF00 | |||
| #define CODE_ALIGNMENT 0x200 | |||
| #define DISPLAY_JUMP_BEHAVIOR DISPLAY_RETAIN_CONTENT | |||
There was a problem hiding this comment.
@TychoVrahe These model_xxx.h headers are starting to get a bit messy. I don't think we need to address it right now, but we could group related symbols together and unify the order of symbols across all these files. Maybe in some subsequent PR...
| } | ||
|
|
||
| secbool erase_storage(flash_progress_callback_t progress_cb) { | ||
| _Static_assert(STORAGE_AREAS_COUNT == 2); |
There was a problem hiding this comment.
lets add message to the static asset, below too
|
|
||
| do { | ||
| if (progress_cb) { | ||
| progress_cb(progress, total); |
There was a problem hiding this comment.
it might be slightly more appropriate to report after the progress calculation, otherwise the last progress won't get reported. If we need to report 0 progress probably it should be done before the loops
| #endif | ||
| #if defined(BOARDLOADER) && defined(USE_SD_CARD) | ||
| {.area = &BOOTLOADER_AREA, .mpu_mode = MPU_MODE_DEFAULT}, | ||
| // { .area = SPARE_AREAS, .mpu_mode = MPU_MODE_SPARE }, |
There was a problem hiding this comment.
should not be commented out? also its seems to be renamed to UNUSED_AREA
|
|
||
| // Progress callback function called during the flash erase operation. | ||
| // | ||
| // Provress is reported as: (100 * pos) / total [%]. |
| // Callback is invoked after each sector or page is erased. | ||
| secbool erase_storage(flash_progress_callback_t progress_cb); | ||
|
|
||
| // Erases all flash areas including storage, asssets and firmware. |
| // Copy the arguments onto the applet stack | ||
| void* arg_copy = NULL; | ||
| if (arg != NULL && arg_size > 0) { | ||
| arg_copy = systask_push_data(&applet->task, arg, arg_size); |
There was a problem hiding this comment.
i guess we should check that the arg was pushed successfully
| "MSR MSP, R0 \n" // Set MSP | ||
| #if defined(__ARM_ARCH_8M_MAIN__) || defined(__ARM_ARCH_8M_BASE__) | ||
| "LDR R0, =_sstack \n" | ||
| "ADD R0, R0, #256 \n" // Add safety margin |
There was a problem hiding this comment.
in startup_stage_2.S (and stages 0 and 1) we use safety margin 128 bytes. Should we unify that? Or maybe even, do we need to reset it here? it shouldn't be changed anywhere i think
| "CMP R0, #0 \n" // == 0 if in thread mode | ||
| "ITTT EQ \n" | ||
| "MOVEQ R0, R5 \n" // R0 = pminfo | ||
| "LDREQ LR, =reboot \n" |
There was a problem hiding this comment.
reboot is called if, for some reason, error_handler returns. do i understand correctly?
| #define HardFault_IRQn (-13) // not defined in stm32lib/cmsis/stm32429xx.h | ||
| #endif | ||
|
|
||
| const char* system_fault_message(const system_fault_t* fault) { |
There was a problem hiding this comment.
the ifdefs in this function make it virtually unreadable. maybe we can ifdef the whole function instead
| "MOV R2, R0 \n" // | ||
| "B 2f \n" // | ||
|
|
||
| "1: \n" |
There was a problem hiding this comment.
could we use better labels then 1,2,3 in here?
This PR introduces a significant conceptual change to our Trezor firmware. It divides the firmware into two parts: a privileged and an unprivileged section. We refer to these as the
kerneland thecoreapp.The kernel includes all hardware drivers, storage management, and necessary cryptographic functions. It operates in privileged mode, providing interfaces to the less trusted coreapp via approximately 100 syscalls.
The coreapp contains the MicroPython core, MicroPython applications, and the Rust-based UI. It runs entirely in unprivileged mode, with no direct access to hardware, except for DMA2D in its current implementation.
Kernelandcoreappare built as two separate applications but eventually glued together by build scripts as a single binary. So you canmake firmwareas before with no change at the first glance.Benefits:
Known issues:
These issues need to be resolved soon, but they do not have a significant impact on security at the moment, as they have not been addressed in the firmware until now.
This PR brings also a lot of changes and improvements to drivers code:
systickdriver that offer more precise time measurement.systimerfor scheduling timer callbacks for background operations.i2c_busdriver, enabling non-blocking i2c operationsmpudriver for region banking, that overcomes cortex-m cpu limitations.systaskmodule, which allows manuall switching between privileged and unprivileged taskssystemmodule with more reliable error handling (emergency mode)