Implement ota and seclink adaption about BK7239N#7146
Implement ota and seclink adaption about BK7239N#7146ewoodev merged 9 commits intoSamsung:BK7239N_Supportfrom
Conversation
| /* Ensure f_priv is cleared even if _files_close didn't clear it | ||
| * (e.g., when filep2->f_inode was NULL) | ||
| */ | ||
| filep2->f_priv = NULL; |
There was a problem hiding this comment.
@Taejun-Kwon @gidori98 Please review fs changes.
| @@ -292,7 +292,8 @@ | |||
| * erased state of the MTD cell */ | |||
| #define MTDIOC_ERASESECTORS _MTDIOC(0x0006) /* IN: Pointer to mtd_erase_s structure | |||
| * OUT: None */ | |||
|
|
|||
| #define MTDIOC_OTATESTBASE _MTDIOC(0x0007) /* IN: None | |||
There was a problem hiding this comment.
It seems to necessary ioctl to read the headers of common and app1 when encryption is enabled.
But I also curious why it needs. If calling MTDIOC_OTATESTBASE ioctl, s_ota_test_flag is set, and read flash directly without flash instruction.
@CYG1124 Why this process is needed? If it is necessary process, then we need to fix binary manager process too.
There was a problem hiding this comment.
Hello @jylee9613
Samsung's OTA test process is as follows:
first, copy a specified length of the image from the flash execution area (e.g., Area A) to the flash area to be activated (e.g., Area B).
During this process, the size and version number of the image to be copied must be read from the head in the flash execution area (Area A); (for non-encrypted images, this process is fine).
However, for encrypted images, the image stored in the flash execution area (Area A) is encrypted, so the head information read directly is encrypted (the image size and version number are encrypted), meaning that the obtained image size and version number are incorrect, which will ultimately affect the data copy.
Thanks
There was a problem hiding this comment.
This is not allowed.
MTDIOC need to be defined if any ioctl is related to mtd operation.
OTA test is not part of mtd operation (struct mtd_dev_s)
So to test OTA in application, you should use open / read / write of block driver.
MTD Layer is totally hided from application.
There was a problem hiding this comment.
Hello @Taejun-Kwon
For OTA testing of encrypted-only images, we need the upper layer to pass a status value to the lower-layer flash driver to distinguish whether the data read from the flash is plaintext or ciphertext. May I ask if there is a better way to pass parameters from the application layer to the lower layer?
ssize_t bk_flash_read(size_t addr, void *buf, size_t length)
{
// printf("func :%s, line :%d, addr :%x, length :%d\n", func, LINE, addr, length);
#if (!CONFIG_SPE)
if (bk_addr_is_kernel(addr)) {
bk_security_flash_read_bytes(addr, (uint8_t *)buf, length);
SCB_CleanInvalidateDCache();
}
#if CONFIG_BUILD_PROTECTED
#if CONFIG_FLASH_ENCRYPT_ENABLE
else if (bk_addr_is_app_or_common(addr)) {
if(s_ota_test_flag) {
bk_data_read_app_or_common(addr, (uint8_t *)buf, length);
} else {
bk_instruction_read_app_or_common(addr, (uint8_t *)buf, length);
}
}
#endif
#endif
else
#endif
{
bk_flash_read_bytes(addr, (uint8_t *)buf, length);
}
return BK_OK;
}
Thanks
There was a problem hiding this comment.
Hello @jylee9613 and @Taejun-Kwon
- Because there is an issue in Samsung's OTA test code regarding the upgrade of encrypted images, it cannot upgrade encrypted images.
- To verify the OTA functionality for encrypted images, I made minimal modifications to Samsung's OTA test code to ensure that the Beken encrypted image upgrade function works properly. The ioctl operation is only used in the test code and is not required in the official OTA process.
- Samsung colleagues in India helped verify the upgrade of encrypted images, and this modification solution is fine.
- If you assess that this modification is not suitable for merging, we can revert the OTA test code changes. Samsung colleagues can maintain this OTA modification patch, and apply it when testing the encrypted OTA functionality.
Thanks
There was a problem hiding this comment.
@ziliguo - You can remove OTA related changes from this PR in TizenRT test code. We will raise another PR for that after discussing with @jylee9613
| @@ -241,6 +241,14 @@ def make_user_binary_header(): | |||
| file_size = fp.tell() | |||
| fp.close() | |||
|
|
|||
| if util.get_value_from_file(cfg_path, "CONFIG_ARCH_CHIP_ARMINO=").rstrip('\n') != 'None': | |||
There was a problem hiding this comment.
Looks good to me. It is necessary to align header with 32-bytes if apply flash encryption.
| @@ -33,30 +33,37 @@ | |||
| os.makedirs(output_folder) | |||
|
|
|||
| CONFIG_ARCH_BOARD = util.get_value_from_file(cfg_file, "CONFIG_ARCH_BOARD=").rstrip('\n') | |||
| board_name = CONFIG_ARCH_BOARD[1:-1] | |||
|
@aashish-l Could you review fs changes? |
|
|
||
| #define CHECKSUM_SIZE 4 | ||
| #define BUFFER_SIZE 512 | ||
| #define USER_SIGN_APPEND_SIZE 32 |
There was a problem hiding this comment.
Unused value. Please remove it.
| } | ||
| #endif | ||
|
|
||
| #if CONFIG_FLASH_ENCRYPT_ENABLE |
There was a problem hiding this comment.
Why is this condition needed?
CONFIG_USER_SIGN_PRETEND_SIZE is related to signature, and it's not related to encryption.
There was a problem hiding this comment.
Hello @jylee9613
1、This condition is required when testing encrypted image upgrades;
2、moreover, encrypted images must be signed beforehand.
Thanks
There was a problem hiding this comment.
About 2、moreover, encrypted images must be signed beforehand.
I heard that the signature is verified before decrypting the image. Then it means that image should be encrypted first and signature is created after encryption.
So I understood that signature header is not encrypted.
If I misunderstood something, please let me know.
There was a problem hiding this comment.
Hello @jylee9613
-
According to Samsung's process, the secure image creation process is as follows:
(1) Steps to create a secure kernel image: First, sign the kernel image (and add the signature header), then encrypt the signed kernel image, and finally add Samsung's kernel_head (16 bytes) to the encrypted kernel image;
(2) Steps to create secure common/app1/app2 images: First, add Samsung's common_head/user_head to the common/app1/app2 images, then sign the images with the Samsung head added (and add the signature header); finally, encrypt the signed common/app1/app2 images;
-
In general: the process of creating secure images is to first complete the signing, and then encrypt the signed images; the verification process is the reverse, first decrypting and then verifying the signature. Since the decryption process is automatically performed by the hardware, it is transparent to the software, so what is observed is that the verification process is executed first;
Thanks
|
|
||
| #ifdef CONFIG_BINARY_SIGNING | ||
| if ((strcmp(binary_info->name, "kernel") != 0)) { | ||
| ret = lseek(read_fd, CONFIG_USER_SIGN_PREPEND_SIZE, SEEK_SET); |
There was a problem hiding this comment.
Is kernel signature header size is same as app1/common?
Actually, CONFIG_USER_SIGN_PREPEND_SIZE is not intended for kernel.
There was a problem hiding this comment.
hello @jylee9613 :
1、kernel signature header size is different form app1/common;
2、yes ,you're right .CONFIG_USER_SIGN_PREPEND_SIZE is not intended for kernel,just for common/app1/app2;
3、so , i use if ((strcmp(binary_info->name, "kernel") != 0)) control it ;
Thanks
There was a problem hiding this comment.
Oh I missed !=... Thank you for explaination!
| } | ||
| #endif | ||
|
|
||
| #if CONFIG_FLASH_ENCRYPT_ENABLE |
There was a problem hiding this comment.
This uses #if instead of #ifdef. Could you confirm this config is defined 0 or 1? I mean this should be defined always regardless of the value. Then, #if is right.
There was a problem hiding this comment.
Hello @sunghan-chang ,
Thanks for your suggestion. The #ifdef is better. I will update it now.
By the way, for unencrypted firmware, this CONFIG_FLASH_ENCRYPT_ENABLE macro is 'n'; for encrypted firmware, this macro is 'y'.
| if util.get_value_from_file(cfg_path, "CONFIG_ARCH_CHIP_ARMINO=").rstrip('\n') != 'None': | ||
| padding_size = 32 - ((header_size + file_size + 4) % 32) | ||
| print("Beken user binary header padding") | ||
| print("crc_size: %d, header_size: %d, file_size: %d, padding_size: %d" % (4, header_size, file_size, padding_size)) | ||
| if padding_size > 0: | ||
| data += b'\xff' * padding_size | ||
| file_size += padding_size | ||
|
|
There was a problem hiding this comment.
Why these paddings are needed in both common and app? Is it related to flash encryption?
To encrypt/decrypt the image with header?
There was a problem hiding this comment.
Hello @jylee9613
Regarding this issue, it is because our hardware encryption and decryption require 32-byte alignment. Therefore, before signing the image, it needs to be aligned to 32 bytes, so that during encryption it can be encrypted directly without needing any additional padding.
Thanks
| ST_EXPECT_EQ(SECLINK_OK, sl_generate_key(g_hnd, HAL_KEY_ED_25519, SL_TEST_ECDH_KEY_SLOT_A)); | ||
|
|
||
| ST_EXPECT_EQ(SECLINK_OK, sl_generate_key(g_hnd, HAL_KEY_ECC_25519, SL_TEST_ECDH_KEY_SLOT_B)); | ||
| ST_EXPECT_EQ(SECLINK_OK, sl_generate_key(g_hnd, HAL_KEY_ED_25519, SL_TEST_ECDH_KEY_SLOT_B)); |
There was a problem hiding this comment.
Please revert this change for x25519 test.
x25519 uses montgomery curve, so HAL_KEY_ECC_25519 is correct key type.
There was a problem hiding this comment.
Please revert this change for
x25519test.x25519uses montgomery curve, soHAL_KEY_ECC_25519is correct key type.
we will rever it. @jylee9613
| ST_EXPECT_EQ(SECLINK_OK, sl_get_key(g_hnd, HAL_KEY_ED_25519, SL_TEST_ECDH_KEY_SLOT_A, &g_key_a)); | ||
|
|
||
| ST_EXPECT_EQ(SECLINK_OK, sl_get_key(g_hnd, HAL_KEY_ECC_25519, SL_TEST_ECDH_KEY_SLOT_B, &g_key_b)); | ||
| ST_EXPECT_EQ(SECLINK_OK, sl_get_key(g_hnd, HAL_KEY_ED_25519, SL_TEST_ECDH_KEY_SLOT_B, &g_key_b)); |
There was a problem hiding this comment.
Same as above (please revert this change)
There was a problem hiding this comment.
Same as above (please revert this change)
we will rever it. @jylee9613
| @@ -568,7 +568,9 @@ TESTCASE_SETUP(ed25519_sign) | |||
| hal_data pub_key = {&g_ed25519_pubkey, sizeof(g_ed25519_pubkey), NULL, 0}; | |||
| hal_data priv_key = {&g_ed25519_privkey_only, sizeof(g_ed25519_privkey_only), NULL, 0}; | |||
|
|
|||
| ST_EXPECT_EQ(0, sl_set_key(g_hnd, HAL_KEY_ECC_25519, RW_SLOT_ENTRY, &pub_key, &priv_key)); | |||
| ST_EXPECT_EQ(0, sl_set_key(g_hnd, HAL_KEY_ED_25519, RW_SLOT_ENTRY, &pub_key, &priv_key)); | |||
| sl_test_print_buffer(g_ed25519_pubkey, sizeof(g_ed25519_pubkey), "yxt ed25519 pubkey"); | |||
| sl_test_print_buffer(g_ed25519_privkey_only, sizeof(g_ed25519_privkey_only), "yxt ed25519 privkey"); | |||
There was a problem hiding this comment.
Same as above (please revert this change)
There was a problem hiding this comment.
Same as above (please revert this change)
I understand that this place needs to be changed to "HAL_KEY_ED_25519". Please confirm.
I will delete this debugging log, thanks @jylee9613
There was a problem hiding this comment.
Hello @jylee9613 ,
I have a question:
for tp1x, you asked to change the key from HAL_KEY_ECC_25519 -> HAL_KEY_ED_25519
when we change the key, the test are passed in realtek.
So why do we have to change for tp1x plus ? shouldn't it be same for both the boards ?
I think this change should be for sign and verify api's only. please correct me if I am wrong.
There was a problem hiding this comment.
Same as above (please revert this change)
I understand that this place needs to be changed to "HAL_KEY_ED_25519". Please confirm. I will delete this debugging log, thanks @jylee9613
I'm sorry this is my mistake.. ed25519_sign need to use HAL_KEY_ED_25519. Thank you!
There was a problem hiding this comment.
Same as above (please revert this change)
I understand that this place needs to be changed to "HAL_KEY_ED_25519". Please confirm. I will delete this debugging log, thanks @jylee9613
I'm sorry this is my mistake.. ed25519_sign need to use HAL_KEY_ED_25519. Thank you!
Done, thank you for confirming.
There was a problem hiding this comment.
Hello @jylee9613 , I have a question: for tp1x, you asked to change the key from HAL_KEY_ECC_25519 -> HAL_KEY_ED_25519 when we change the key, the test are passed in realtek. So why do we have to change for tp1x plus ? shouldn't it be same for both the boards ? I think this change should be for sign and verify api's only. please correct me if I am wrong.
I asked to change HAL_KEY_ECC_25519 to HAL_KEY_ED_25519 only for ed25519 sign/verify.
TP1x already passed the testcase for x25519 sign/verify because the key type was HAL_KEY_ECC_25519.
But ed25519 sign/verify returns HAL_BAD_KEY_PAIR in TP1x because of wrong key type.
There was a problem hiding this comment.
No, it's not same.
x25519_compute uses random key generated by sl_generate_key
|
|
||
| #define CHECKSUM_SIZE 4 | ||
| #define BUFFER_SIZE 512 | ||
| #define USER_SIGN_APPEND_SIZE 32 |
There was a problem hiding this comment.
Got it. It is Done now.
| @@ -292,7 +292,8 @@ | |||
| * erased state of the MTD cell */ | |||
| #define MTDIOC_ERASESECTORS _MTDIOC(0x0006) /* IN: Pointer to mtd_erase_s structure | |||
| * OUT: None */ | |||
|
|
|||
| #define MTDIOC_OTATESTBASE _MTDIOC(0x0007) /* IN: None | |||
There was a problem hiding this comment.
This is not allowed.
MTDIOC need to be defined if any ioctl is related to mtd operation.
OTA test is not part of mtd operation (struct mtd_dev_s)
So to test OTA in application, you should use open / read / write of block driver.
MTD Layer is totally hided from application.
| /* Ensure f_priv is cleared even if _files_close didn't clear it | ||
| * (e.g., when filep2->f_inode was NULL) | ||
| */ | ||
| filep2->f_priv = NULL; |
| @@ -461,6 +467,7 @@ void files_release(int fd) | |||
| list->fl_files[fd].f_oflags = 0; | |||
| list->fl_files[fd].f_pos = 0; | |||
| list->fl_files[fd].f_inode = NULL; | |||
| list->fl_files[fd].f_priv = NULL; | |||
| with open(output_folder + "app1_" + str(app1_ota_idx) + ".ld", "w") as ld : | ||
| print("OTA index in app1 ", app1_ota_idx) | ||
| ld.write(ld_script_content) | ||
| if is_rtl_mode: |
There was a problem hiding this comment.
Could you please let me know why it is needed?
What is difference or what point is sepecific chipset?
There was a problem hiding this comment.
this changes are for support only single ota ld script? (not dual A/B) with flash controller dynanic addressing
There was a problem hiding this comment.
Hello @ewoodev :
-
Because BEKEN's XIP solution supports position-independent upgrades, for common: only one ld file (common_0.ld) needs to be generated, so the compiled common_bk7239n_200204.trpk binary can be executed in both common region A and common region B; the same applies to app2 and app1 (only one app1_0.ld and app2_0.ld need to be generated).
-
However, when compiling the RTL8730E XIP project, two ld files are generated (common_0.ld & common_1.ld), and the same goes for app2 and app1 (app1_0.ld & app1_1.ld and app2_0.ld & app2_1.ld will be generated).
-
Therefore, I optimized the mkldscript.py script to support both BEKEN's XIP OTA upgrade solution and RTL8730E's XIP OTA upgrade solution.
Thanks
There was a problem hiding this comment.
Thank you for improve script
we can have sevel chipset.
Could you please just rename is_dual_ld_mode? is_dual_image_mode?
There was a problem hiding this comment.
There was a problem hiding this comment.
Hello @ewoodev ,
It is Done now.
We have changed it as "is_dual_ld_mode". PLease help check if anything is missed. Thank you.
f54ce2c to
43e515f
Compare
| if board_name == "rtl8730e": | ||
| sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '../../build/tools/amebasmart/gnu_utility'))) | ||
| from loadable_xip_elf import get_offset | ||
| offset_shift = get_offset() | ||
| offset = int(CONFIG_FLASH_VSTART_LOADABLE, 16) - int(offset_shift, 16) | ||
| loadable_start_offset = offset | ||
| elif CONFIG_ARCH_BOARD[1:-1] == "bk7239n": | ||
| loadable_start_offset = int(CONFIG_FLASH_VSTART_LOADABLE, 16) - int(offset_shift, 16) | ||
| elif board_name == "bk7239n": | ||
| loadable_start_offset = int(CONFIG_FLASH_VSTART_LOADABLE, 16) | ||
| offset = loadable_start_offset | ||
| else: | ||
| # For other boards, use CONFIG_FLASH_VSTART_LOADABLE directly | ||
| loadable_start_offset = int(CONFIG_FLASH_VSTART_LOADABLE, 16) | ||
| offset = loadable_start_offset |
There was a problem hiding this comment.
How about set is_dual_ld_mode here?
because already this is chipset sepecific code. we want to minimize chipset sepecific code in this file.
There was a problem hiding this comment.
- clean source code of previous chipset - achieve location independence for app and commonfirmware - remove unused log - optimize the ota test code - fix reboot reason test fail issue - fix adv/conn ind report issue - adapt the ed25519 with libsodium
- fix ota issue - adaption for seclink test
- clean project of previous chipset - enable reboot reason test case for loadapp
- beken tfm prehandle partition python script update
- fix tick compensation issue during interrupt disbale period, with the RTC device - update the flash adaption code to optimize the ota test code
… case - In concurrent scenarios (such as the UART test and iperf running concurrently in Scenario 3), the file descriptor may be in an inconsistent state(f_priv has a value but f_inode is NULL), leading to the ASSERT at os/fs/smartfs/smartfs_smart.c:line887 problem. - Problem Flow: file_dup2 calls _files_close(filep2) (line 313) _files_close only cleans up f_priv if f_inode != NULL (lines 132-150) If filep2->f_inode is NULL, f_priv is not cleaned up. Subsequently, filep2->f_inode is set to inode (line 329) When smartfs_dup is called (line 340), the assertion fails because newp->f_priv != NULL.
- use board_name to distingush different board type - add padding for common partition
- adapt for ed25519 of security wrapper
- clean useless projects for beken BK7239N chipset in circleci/config.yml
|
@amandeep-samsung Could you leave test result here? |
|
| # For dual mode, initialize offset with loadable_start_offset | ||
| # For generic mode, offset starts from 0 and will be reset when encountering first loadable partition | ||
| if is_dual_ld_mode: | ||
| offset = loadable_start_offset |
There was a problem hiding this comment.
In previous code, for every case offset is same with loadable_start_offset.
Is there any reason for excute offset = loadable_start_offset only for is_dual_ld_mode.
In line 58, even if not dual_ld_mode, we're setting offset = loadable_start_offset.
There was a problem hiding this comment.
@amandeep-samsung
Could you please consider in sepearted your new PR?
There was a problem hiding this comment.
@amandeep-samsung Could you please consider in sepearted your new PR?
Thank you for your suggestion. We will update it in the new PR later.
| if not ld_generated["common"] and CONFIG_SUPPORT_COMMON_BINARY == 'y': | ||
| common_start = hex(offset + 0x10 + signing_offset) | ||
| common_size = hex(part_size - 0x10 - signing_offset) | ||
| with open(output_folder + "common_0.ld", "w") as ld : |
There was a problem hiding this comment.
How about print log like app1 and app2.
There was a problem hiding this comment.
@amandeep-samsung
Could you please consider in sepearted your new PR?
There was a problem hiding this comment.
@amandeep-samsung Could you please consider in sepearted your new PR?
We will add it in the new PR later.
jylee9613
left a comment
There was a problem hiding this comment.
All security features confirmed except OTA. (It'll be updated to next PR)
1、Ota adaption to support a single binary file executes in both partition_A and partition_B;
2、Adapt the ed25519 for sec link test;
3、Clean driver code of previous chipset;
4、Tick compensation improvment during interrupt disabled as required;
5、Some bug fix and modification according to the comments of PR review.
-Resolved the 5 issues of previous PR7137:
-Notice:
BK7239N has 4 projects currently. All four projects below are for MP(mass production) chipset. The two projects for previous chipset have been removed in this PR.
"1. xip_all_mp" ==> project for MP chipset. Option C: TFM/Kernel/Common/APP1/APP2 runs on Flash;
"2. loadable_apps_mp" ==> project for MP chipset. Option B: TFM/Kernel runs on Flash, Common/APP1/APP2 runs on PSRAM;
"3. hello_mp" ==> project for MP chipset. Flat build;
"4. loadable_all_mp" ==> project for MP chipset. Option D: TFM/Kernel/Common/APP1/APP2 runs on PSRAM.