Adding code to clear the pkcs#11 persistent db after deinit call so that memory leaks are not observed. #7658

sahesaha · 2025-12-17T07:39:58Z

Tested and verified that there is no PKCS#11 TA crash observed due to memory leak.
The persistent_db is cleared after deinit() call.

Reviewed-by: Neeraj Soni [email protected]
Saurabh Gupta [email protected]
Tested on: SM7325 SoC
Signed-off-by: Saheli Saha [email protected]

saursaur-oss · 2025-12-17T09:49:01Z

ta/pkcs11/src/persistent_token.c

 /*
 * Release resources relate to persistent database
 */
 void close_persistent_db(struct ck_token *token __unused)


please remove ___unused keyword.

jforissier

Hi @sahesaha,

Please see my comments below. Also, please make the subject "ta: pkcs11: close_persistent_db() should free memory".

Can you elaborate on how you observed a TA crash? It seems thatclose_persistent_db() is only called as a result of TA_DestroyEntryPoint() -> pkcs11_deinit() so the TA should be gone after that point?

jforissier · 2025-12-17T09:45:26Z

ta/pkcs11/src/persistent_token.c

+		return;
+
+	/* Free the database main structure if allocated */
+	if (token->db_main) {


This check is not needed because TEE_free(NULL) is valid

jforissier · 2025-12-17T09:45:57Z

ta/pkcs11/src/persistent_token.c

+	if (!token)
+		return;
+
+	/* Free the database main structure if allocated */


Obvious comment, please remove

etienne-lms

close_persistent_db() is only called from pkcs11_deinit() that is only called from TA_DestroyEntryPoint() which is called when the TA is unloaded from RAM hence at a stage when, whatever happens, the TA heap is destroyed. Therefore, it is not needed to free allocations in the heap in these function.

close_persistent_db() has been implemented as an empty function only for consistency, as a balance to init_persistent_db(), in case there would be, in the future, some persistent data or system resources to be released.

Could you confirm this P-R fixes a heap leakage issue?

sahesaha · 2026-01-09T12:46:09Z

@saursaur-oss @jforissier @etienne-lms. Addressed the comments.
It has been observed that token->db_main and token->db_objs is not cleared after deinit() call when the TA is unloaded. Memory leak has been observed in the shutdown sequence of pkcs11 ta and this PR fixes the leak.

etienne-lms

Sorry to insist, how did you measure or observe this heap memory pool leakage issue? I would expect after pkcs11_deinit() is called, the whole TA is unloaded from RAM so allocations in its heap cannot remain. Loading back the TA would make it start from a fresh clear/clean brand new heap memory pool.

Note that pkcs11 has TA_FLAG_INSTANCE_KEEP_ALIVE flag set hence it never unloaded, pkcs11_deinit() should never be called. Actually it's unloaded when it crashes, in which case deinit() is not called. That said, one can still build the TA with this flag clear so that it is unloaded when there are no pending sessions toward that TA (sessions are opened/closed respectively by Cryptoki C_Initialize()/C_Finalize() API functions).

etienne-lms · 2026-01-09T17:06:37Z

ta/pkcs11/src/pkcs11_attributes.c

 		 */
 		return get_bool(obj->attributes, req_attr->id);
+	case PKCS11_CKA_OPTEE_INDESTRUCTIBLE:
+		return false;


Typo: unrelated to this P-R.

Tested and verified that there is no PKCS#11 TA crash observed due to memory leak. The persistent_db is cleared after deinit() call. Reviewed-by: Neeraj Soni <[email protected]> Reviewed-by: Saurabh Gupta <[email protected]> Tested on: SM7325 SoC Signed-off-by: Saheli Saha <[email protected]>

sahesaha · 2026-01-12T10:42:49Z

Addressed the typo. @saursaur-oss can help regarding the discussion for heap memory leak.

saursaur-oss · 2026-01-12T10:45:01Z

Sorry to insist, how did you measure or observe this heap memory pool leakage issue? I would expect after pkcs11_deinit() is called, the whole TA is unloaded from RAM so allocations in its heap cannot remain. Loading back the TA would make it start from a fresh clear/clean brand new heap memory pool.

Note that pkcs11 has TA_FLAG_INSTANCE_KEEP_ALIVE flag set hence it never unloaded, pkcs11_deinit() should never be called. Actually it's unloaded when it crashes, in which case deinit() is not called. That said, one can still build the TA with this flag clear so that it is unloaded when there are no pending sessions toward that TA (sessions are opened/closed respectively by Cryptoki C_Initialize()/C_Finalize() API functions).

During TA shutdown, we verify that all memory allocated from pools has been properly freed. If any memory remains used, it indicates that some pools are still active. Simply removing the TA from RAM allocation does not perform memory integrity checks—it only clears the TA’s memory footprint from RAM.

etienne-lms

I still not convinced this change is required. At least it makes the database init/close sequence more consistent but there remain other buffers to free.

etienne-lms · 2026-01-12T10:54:07Z

ta/pkcs11/src/persistent_token.c

+	TEE_Free(token->db_main);
+	token->db_main = NULL;
+
+	TEE_Free(token->db_objs);


What if there are persistent objects in the database? They would still have related resources allocated in the heap. token->db_objs->uuids array stores token->db_objs->count pointers of UUIDs allocated in the heap. token->object_list holds a list of all those objects for each a struct pkcs11_object instance is allocated in the heap which object may have heap allocated resources (e.g. its attributes field). For consistency, these should also be freed.

saursaur-oss · 2026-01-12T11:49:43Z

I still not convinced this change is required. At least it makes the database init/close sequence more consistent but there remain other buffers to free.

Most data is stored in persistent storage, and the associated memory remains allocated, leading to memory leaks. However, this data(db_main and db_objs) can be reinitialized from storage when needed.

sahesaha force-pushed the Clearing_persistent_db branch 3 times, most recently from 880330d to 05c3324 Compare December 17, 2025 07:58

saursaur-oss suggested changes Dec 17, 2025

View reviewed changes

jforissier reviewed Dec 17, 2025

View reviewed changes

etienne-lms reviewed Jan 8, 2026

View reviewed changes

sahesaha force-pushed the Clearing_persistent_db branch from 05c3324 to 8fb6155 Compare January 9, 2026 12:42

etienne-lms reviewed Jan 9, 2026

View reviewed changes

sahesaha force-pushed the Clearing_persistent_db branch from 8fb6155 to 81fb805 Compare January 12, 2026 10:42

etienne-lms reviewed Jan 12, 2026

View reviewed changes

Adding code to clear the pkcs#11 persistent db after deinit call so that memory leaks are not observed. #7658

Are you sure you want to change the base?

Adding code to clear the pkcs#11 persistent db after deinit call so that memory leaks are not observed. #7658

Conversation

sahesaha commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saursaur-oss Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

jforissier Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

jforissier Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

sahesaha commented Jan 9, 2026

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

sahesaha commented Jan 12, 2026

Uh oh!

saursaur-oss commented Jan 12, 2026

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

saursaur-oss commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sahesaha commented Dec 17, 2025 •

edited

Loading