Add PKCS11 backend for encrypted partitions #653
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds PKCS#11 backend support for encrypted partitions in wolfBoot, enabling the use of wolfPKCS11 as the crypto backend for partition encryption instead of plain wolfCrypt. The implementation allows applications to store encryption keys in the keyvault with a specific ID, which wolfBoot can then retrieve and use for encryption operations.
Key changes include:
- Added new
ENCRYPT_PKCS11configuration option with support for PKCS#11-based encryption - Implemented PKCS11 crypto functions (init, encrypt, decrypt, set_iv, deinit) in
src/libwolfboot.c - Fixed variable scoping issues for
sel_secto only declare it whenNVM_FLASH_WRITEONCEis defined - Updated build configuration in
options.mkto handle PKCS11 encryption alongside existing AES and ChaCha options
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/update_flash.c | Moved WP11_Library_Init() call earlier in boot sequence and added pkcs11_crypto_deinit() cleanup call |
| src/libwolfboot.c | Added complete PKCS11 crypto implementation with init/deinit/encrypt/decrypt functions; fixed variable scoping for sel_sec |
| options.mk | Added PKCS11 encryption configuration logic with mechanism selection and parameter definitions; updated AES object inclusion logic |
| include/wolfboot/wolfboot.h | Added ENCRYPT_PKCS11 macro definitions for block size, key size, and nonce size |
| include/user_settings.h | Added conditional compilation guards for AES settings; removed direct ENCRYPT_WITH_AES128 definition |
| include/encrypt.h | Added PKCS11 crypto function declarations and macro definitions |
| docs/encrypted_partitions.md | Added comprehensive documentation for PKCS#11 backend configuration and usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = pkcs11_function_list->C_OpenSession(1, | ||
| CKF_SERIAL_SESSION | CKF_RW_SESSION, NULL, NULL, | ||
| &pkcs11_session); | ||
| session_opened = 1; |
There was a problem hiding this comment.
The session_opened flag is set to 1 immediately after calling C_OpenSession, but before checking if the call succeeded. This means that if C_OpenSession fails but subsequent checks pass (which is impossible in this flow, but logically incorrect), the error cleanup could attempt to close a session that was never successfully opened. Move the session_opened = 1 assignment inside a check that ret == CKR_OK after the C_OpenSession call.
| session_opened = 1; | |
| if (ret == CKR_OK) { | |
| session_opened = 1; | |
| } |
| if (ret == CKR_OK) { | ||
| ret = pkcs11_function_list->C_Login(pkcs11_session, CKU_USER, | ||
| pkcs11_pin, sizeof(pkcs11_pin) - 1); | ||
| logged_in = 1; |
There was a problem hiding this comment.
The logged_in flag is set to 1 immediately after calling C_Login, but before checking if the call succeeded. This means that if C_Login fails, the error cleanup will incorrectly attempt to call C_Logout on a session that was never successfully logged in. Move the logged_in = 1 assignment inside a check that ret == CKR_OK after the C_Login call.
| logged_in = 1; | |
| if (ret == CKR_OK) { | |
| logged_in = 1; | |
| } |
| } | ||
|
|
||
| if (ret == CKR_OK && obj_count != 1) { | ||
| ret = -1; |
There was a problem hiding this comment.
When the key is not found (obj_count != 1), ret is set to -1 instead of a proper CK_RV error code. This inconsistency could cause issues with error handling. Consider using a standard PKCS#11 error code like CKR_KEY_HANDLE_INVALID or CKR_OBJECT_HANDLE_INVALID instead of -1 to maintain consistency with the rest of the PKCS#11 API error handling.
| ret = -1; | |
| ret = CKR_OBJECT_HANDLE_INVALID; |
|
|
||
| pkcs11_enc_initialized = 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. Please remove it.
| void pkcs11_crypto_deinit(void) | ||
| { | ||
| if (encrypt_initialized) { | ||
| pkcs11_function_list->C_CloseSession(pkcs11_session); | ||
| encrypt_initialized = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
The pkcs11_crypto_deinit function should call C_Finalize to properly clean up the PKCS#11 library before closing the session. According to the PKCS#11 specification, C_Finalize should be called to indicate that an application is done with the Cryptoki library. Add a call to pkcs11_function_list->C_Finalize(NULL) before closing the session.
There was a problem hiding this comment.
pkcs11_crypto_deinit is just meant to close the session opened by wolfBoot in order to make wolfPKCS11 ready for future calls by the app (as opposed to wolfBoot). We're not really "done with the Cryptoki library".
| } | ||
| if (session_opened) { | ||
| pkcs11_function_list->C_CloseSession(pkcs11_session); | ||
| } |
There was a problem hiding this comment.
When C_Initialize succeeds but subsequent operations fail, the function should call C_Finalize before returning to properly clean up the PKCS#11 library state. Currently, only C_Logout and C_CloseSession are called on error. Consider adding pkcs11_function_list->C_Finalize(NULL) to the error cleanup path.
| } | |
| } | |
| pkcs11_function_list->C_Finalize(NULL); |
| if (pkcs11_enc_initialized) { | ||
| ret = pkcs11_function_list->C_EncryptFinal(pkcs11_session, buf, | ||
| &buf_len); | ||
| if (ret != CKR_OK) { | ||
| return; | ||
| } | ||
| pkcs11_enc_initialized = 0; | ||
| } | ||
| else if (pkcs11_dec_initialized) { | ||
| ret = pkcs11_function_list->C_DecryptFinal(pkcs11_session, buf, | ||
| &buf_len); | ||
| if (ret != CKR_OK) { | ||
| return; | ||
| } | ||
| pkcs11_dec_initialized = 0; | ||
| } |
There was a problem hiding this comment.
When C_EncryptFinal or C_DecryptFinal fails, the function returns early without properly resetting the initialization flags. This could leave the system in an inconsistent state. Consider ensuring pkcs11_enc_initialized or pkcs11_dec_initialized is set to 0 even when the Final call fails, or add error logging to help diagnose the failure.
2 participants
This basically adds
ENCRYPT_PKCS11and a few new options which make wolfBoot use wolfPKCS11 as the crypto backend for partition encryption (rather than plain wolfCrypt), let the application store the encryption key in the keyvault with a specific ID and make that ID available to wolfBoot. More info in the changes todocs/encrypted_partitions.md.