From c9e25c3e2dd6c524f1fc2e89eeba54d1178f87ac Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 16 Oct 2026 12:41:12 +0100 Subject: [PATCH 1/3] Write only changed token objects to storage Store ids are list positions, and C_CreateObject and C_DestroyObject rewrote every token object. With a flash backed custom store, such as wolfBoot's, each write got slower as the token grew. A create now writes the new object and the token record. A destroy moves the newest object into the freed position, so at most that one object is rewritten. C_SetAttributeValue writes the changed token object, as the next create or destroy no longer does. --- src/crypto.c | 3 + src/internal.c | 207 ++++++++---- tests/include.am | 7 + tests/token_partial_store_test.c | 535 +++++++++++++++++++++++++++++++ wolfpkcs11/internal.h | 1 + 5 files changed, 699 insertions(+), 54 deletions(-) create mode 100644 tests/token_partial_store_test.c diff --git a/src/crypto.c b/src/crypto.c index b051e9c6..b4537bad 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -2557,6 +2557,9 @@ static CK_RV wp11_C_SetAttributeValue(CK_SESSION_HANDLE hSession, rv = CheckCopyOnlyAttributes(obj, pTemplate, ulCount); if (rv == CKR_OK) rv = SetAttributeValue(session, obj, pTemplate, ulCount, CK_FALSE); + /* Token objects are only rewritten when they change, so write it now. */ + if (rv == CKR_OK && WP11_Object_Persist(obj) != 0) + rv = CKR_FUNCTION_FAILED; WOLFPKCS11_LEAVE("C_SetAttributeValue", rv); return rv; } diff --git a/src/internal.c b/src/internal.c index e34c7a07..02afafde 100644 --- a/src/internal.c +++ b/src/internal.c @@ -9080,17 +9080,30 @@ static int wp11_Token_Load(WP11_Slot* slot, int tokenId, WP11_Token* token) return ret; } +/* Records written by a token store pass. */ +#define WP11_STORE_PASS_ALL 0 /* Token record and every object. */ +#define WP11_STORE_PASS_TOKEN 1 /* Token record and the one object. */ +#define WP11_STORE_PASS_OBJECT 2 /* Only the one object. */ + /** - * Store a token to storage. + * Store a token, or some of its records, to storage. + * + * An object's record is keyed by its position in the token's object list, + * so only objects that are new, modified or moved need to be written. * - * @param [in] object Token object. + * @param [in] token Token object. * @param [in] tokenId Id of token this key belongs to. + * @param [in] what WP11_STORE_PASS_* value. + * @param [in] only Object to store when not storing all of them. May be + * NULL with WP11_STORE_PASS_TOKEN to store only the + * token record. * @return 0 on success. * @return MEMORY_E when dynamic memory allocation fails. * @return BUFFER_E when storing fails. * @return NOT_AVAILABLE_E when unable to write data. */ -static int wp11_Token_StorePass(WP11_Token* token, int tokenId) +static int wp11_Token_StorePass(WP11_Token* token, int tokenId, int what, + WP11_Object* only) { int ret = 0; int i; @@ -9099,36 +9112,46 @@ static int wp11_Token_StorePass(WP11_Token* token, int tokenId) int variableSz; #ifdef WOLFPKCS11_DEBUG_STORE - printf("wp11_Token_Store: tokenId %d, objCnt %d\n", tokenId, token->objCnt); + printf("wp11_Token_Store: tokenId %d, objCnt %d, what %d\n", tokenId, + token->objCnt, what); #endif #ifdef WOLFPKCS11_TPM_STORE - /* Reserve NV space for token object before the objects use it. */ - variableSz = token->userPinLen + token->soPinLen + - (token->objCnt * FIELD_SIZE(WP11_Object, type)); - ret = wp11_storage_open(WOLFPKCS11_STORE_TOKEN, tokenId, 0, variableSz, - &storage); - if (ret == 0) { - ret = wp11_storage_close_write(storage, ret); - storage = NULL; + if (what != WP11_STORE_PASS_OBJECT) { + /* Reserve NV space for token object before the objects use it. */ + variableSz = token->userPinLen + token->soPinLen + + (token->objCnt * FIELD_SIZE(WP11_Object, type)); + ret = wp11_storage_open(WOLFPKCS11_STORE_TOKEN, tokenId, 0, + variableSz, &storage); + if (ret == 0) { + ret = wp11_storage_close_write(storage, ret); + storage = NULL; + } } #endif /* Store the objects */ object = token->object; for (i = token->objCnt - 1; i >= 0; i--) { - /* Write the objects. */ - ret = wp11_Object_Store(object, tokenId, i); - if (ret != 0) { - #ifdef DEBUG_WOLFPKCS11 - printf("Failed to store object %d, token %d, ret: %d\n", - i, tokenId, ret); - #endif - break; + if (what == WP11_STORE_PASS_ALL || object == only) { + /* Write the objects. */ + ret = wp11_Object_Store(object, tokenId, i); + if (ret != 0) { + #ifdef DEBUG_WOLFPKCS11 + printf("Failed to store object %d, token %d, ret: %d\n", + i, tokenId, ret); + #endif + break; + } } object = object->next; } + if (what == WP11_STORE_PASS_OBJECT) { + /* Not writing the token record. */ + return (ret == NOT_AVAILABLE_E) ? 0 : ret; + } + if (ret == 0) { variableSz = token->userPinLen + token->soPinLen + (token->objCnt * FIELD_SIZE(WP11_Object, type)); @@ -9221,14 +9244,17 @@ static int wp11_Token_StorePass(WP11_Token* token, int tokenId) } /** - * Store the token and its objects as one pass that commits as a unit. + * Store records of the token as one pass that commits as a unit. * * @param [in] token Token object. * @param [in] tokenId Id of token. + * @param [in] what WP11_STORE_PASS_* value. + * @param [in] only Object to store when not storing all of them. * @return 0 on success. * @return Other value when the pass did not commit. */ -static int wp11_Token_Store(WP11_Token* token, int tokenId) +static int wp11_Token_StoreSome(WP11_Token* token, int tokenId, int what, + WP11_Object* only) { int ret; #ifdef WP11_STORE_BATCH @@ -9236,17 +9262,30 @@ static int wp11_Token_Store(WP11_Token* token, int tokenId) ret = wolfPKCS11_StoreBatchBegin(tokenId); if (ret == 0) { - ret = wp11_Token_StorePass(token, tokenId); + ret = wp11_Token_StorePass(token, tokenId, what, only); endRet = wolfPKCS11_StoreBatchEnd(ret == 0); if (ret == 0) ret = endRet; } #else - ret = wp11_Token_StorePass(token, tokenId); + ret = wp11_Token_StorePass(token, tokenId, what, only); #endif return ret; } + +/** + * Store the token and all its objects as one pass that commits as a unit. + * + * @param [in] token Token object. + * @param [in] tokenId Id of token. + * @return 0 on success. + * @return Other value when the pass did not commit. + */ +static int wp11_Token_Store(WP11_Token* token, int tokenId) +{ + return wp11_Token_StoreSome(token, tokenId, WP11_STORE_PASS_ALL, NULL); +} #endif /* !WOLFPKCS11_NO_STORE */ /** @@ -9467,7 +9506,8 @@ static int wp11_Slot_Load(WP11_Slot* slot, int id) * @return BUFFER_E when storing fails. * @return NOT_AVAILABLE_E when unable to write data. */ -static int wp11_Slot_Store(WP11_Slot* slot, int id) +static int wp11_Slot_StoreSome(WP11_Slot* slot, int id, int what, + WP11_Object* only) { /* Only persist tokens that have been explicitly provisioned. Otherwise * a C_Initialize / C_Finalize cycle on a never-init'd slot would write @@ -9475,7 +9515,12 @@ static int wp11_Slot_Store(WP11_Slot* slot, int id) * load and treat as INITIALIZED (Fenrir 3407 regression). */ if (slot->token.state != WP11_TOKEN_STATE_INITIALIZED) return 0; - return wp11_Token_Store(&slot->token, id); + return wp11_Token_StoreSome(&slot->token, id, what, only); +} + +static int wp11_Slot_Store(WP11_Slot* slot, int id) +{ + return wp11_Slot_StoreSome(slot, id, WP11_STORE_PASS_ALL, NULL); } #endif @@ -12661,7 +12706,10 @@ int WP11_Session_AddObject(WP11_Session* session, int onToken, } #ifndef WOLFPKCS11_NO_STORE if (ret == 0) { - ret = wp11_Slot_Store(session->slot, (int)session->slotId); + /* The new object goes at the head of the list and takes the next + * store id, so the records of the other objects are unchanged. */ + ret = wp11_Slot_StoreSome(session->slot, (int)session->slotId, + WP11_STORE_PASS_TOKEN, object); if (ret != 0) { token->object = oldHead; token->objCnt = oldObjCnt; @@ -12834,34 +12882,29 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, } if (onToken) { + WP11_Object* head = token->object; + WP11_Object* moved = NULL; + + /* Store id of an object is its position counted from the end of the + * list, so the head has the highest id. Walk to the object to remove + * to get its id. */ + id = token->objCnt - 1; + for (curr = &token->object; *curr != object; curr = &(*curr)->next) + id--; + *curr = object->next; token->objCnt--; - /* Id of first object on token. */ - id = token->objCnt; - curr = &token->object; - /* walk list to get id for object to remove */ - while (*curr != NULL) { - if (*curr == object) { - *curr = object->next; - break; - } - #ifndef WOLFPKCS11_NO_STORE - /* remove any id's with higher value */ - storeRet = wp11_Object_Unstore(*curr, (int)session->slotId, id); - #ifdef DEBUG_WOLFPKCS11 - if (storeRet != 0) { - printf("Failed to unstore slot %d, id: %d, ret: %d, continuing...\n", - (int)session->slotId, id, storeRet); + /* Removing the object would shift the ids of every newer object down + * by one, and each of them would have to be rewritten. Instead move + * the head into the place of the removed object, so it takes the + * removed object's id and is the only object rewritten. */ + if (object != head) { + moved = head; + if (curr != &head->next) { + token->object = head->next; + head->next = *curr; + *curr = head; } - #endif - /* NOT_AVAILABLE_E: storage is disabled, nothing to remove. */ - if (ret == 0 && storeRet != NOT_AVAILABLE_E) - ret = storeRet; - #endif - - curr = &(*curr)->next; - /* Id of next object as it isn't the one being removed. */ - id--; } #ifndef WOLFPKCS11_NO_STORE @@ -12872,10 +12915,12 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, (int)session->slotId, id, storeRet); } #endif + /* NOT_AVAILABLE_E: storage is disabled, nothing to remove. */ if (ret == 0 && storeRet != NOT_AVAILABLE_E) ret = storeRet; - /* re-store the token */ - storeRet = wp11_Slot_Store(session->slot, (int)session->slotId); + /* Write the moved object under its new id and the token record. */ + storeRet = wp11_Slot_StoreSome(session->slot, (int)session->slotId, + WP11_STORE_PASS_TOKEN, moved); #ifdef DEBUG_WOLFPKCS11 if (storeRet != 0) { printf("Failed to store slot %d, ret: %d\n", @@ -12885,6 +12930,22 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, /* A token record left unwritten outranks a record left behind. */ if (storeRet != 0) ret = storeRet; + if (moved != NULL && storeRet == 0) { + /* Remove the moved object's records under its old id, the + * highest one, which is no longer used. */ + storeRet = wp11_Object_Unstore(moved, (int)session->slotId, + token->objCnt); + #ifdef DEBUG_WOLFPKCS11 + if (storeRet != 0) { + printf("Failed to unstore slot %d, id: %d, ret: %d\n", + (int)session->slotId, token->objCnt, storeRet); + } + #endif + if (ret == 0 && storeRet != NOT_AVAILABLE_E) + ret = storeRet; + } + #else + (void)moved; #endif } else { @@ -13470,6 +13531,44 @@ int WP11_Object_OnToken(WP11_Object* object) return object->onToken; } +/** + * Write a token object to storage after its attributes have changed. + * Only the object's own records are written. Session objects are not stored. + * + * @param object [in] Object object. + * @return 0 on success or when nothing needs storing. + * Other value when storing failed. + */ +int WP11_Object_Persist(WP11_Object* object) +{ + int ret = 0; +#ifndef WOLFPKCS11_NO_STORE + WP11_Slot* slot = object->slot; + WP11_Token* token; + WP11_Object* curr; + + if (!object->onToken || slot == NULL) + return 0; + + token = &slot->token; + WP11_Lock_LockRW(&token->lock); + /* The object may have been destroyed by another session. */ + for (curr = token->object; curr != NULL; curr = curr->next) { + if (curr == object) + break; + } + if (curr != NULL) { + ret = wp11_Slot_StoreSome(slot, (int)slot->id, WP11_STORE_PASS_OBJECT, + object); + } + WP11_Lock_UnlockRW(&token->lock); +#else + (void)object; +#endif + + return ret; +} + /** * Get the object's type - for example the key type. * diff --git a/tests/include.am b/tests/include.am index ff1565f6..5b8111dd 100644 --- a/tests/include.am +++ b/tests/include.am @@ -400,6 +400,11 @@ noinst_PROGRAMS += tests/unwrap_keygen_test tests_unwrap_keygen_test_SOURCES = tests/unwrap_keygen_test.c tests_unwrap_keygen_test_LDADD = +check_PROGRAMS += tests/token_partial_store_test +noinst_PROGRAMS += tests/token_partial_store_test +tests_token_partial_store_test_SOURCES = tests/token_partial_store_test.c +tests_token_partial_store_test_LDADD = + if BUILD_STATIC tests_pkcs11test_LDADD += src/libwolfpkcs11.la tests_pkcs11mtt_LDADD += src/libwolfpkcs11.la @@ -481,6 +486,7 @@ tests_sign_verify_mac_test_LDADD += src/libwolfpkcs11.la tests_digest_length_test_LDADD += src/libwolfpkcs11.la tests_secret_scrub_test_LDADD += src/libwolfpkcs11.la tests_unwrap_keygen_test_LDADD += src/libwolfpkcs11.la +tests_token_partial_store_test_LDADD += src/libwolfpkcs11.la else tests_object_id_uniqueness_test_LDADD += src/libwolfpkcs11.la tests_empty_pin_store_test_LDADD += src/libwolfpkcs11.la @@ -554,6 +560,7 @@ tests_sign_verify_mac_test_LDADD += src/libwolfpkcs11.la tests_digest_length_test_LDADD += src/libwolfpkcs11.la tests_secret_scrub_test_LDADD += src/libwolfpkcs11.la tests_unwrap_keygen_test_LDADD += src/libwolfpkcs11.la +tests_token_partial_store_test_LDADD += src/libwolfpkcs11.la endif EXTRA_DIST += tests/unit.h \ diff --git a/tests/token_partial_store_test.c b/tests/token_partial_store_test.c new file mode 100644 index 00000000..d507d7c4 --- /dev/null +++ b/tests/token_partial_store_test.c @@ -0,0 +1,535 @@ +/* token_partial_store_test.c + * + * Copyright (C) 2006-2026 wolfSSL Inc. + * + * This file is part of wolfPKCS11. + * + * wolfPKCS11 is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfPKCS11 is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + * + * Token objects are written to storage only when they are created, changed + * or moved. This test checks that: + * - creating an object does not rewrite the records of the other objects, + * - after creates, destroys and an attribute change, the records written so + * far load back as the right objects without relying on the full store + * done by C_Finalize, + * - no records are left behind for destroyed objects. + */ + +#ifdef HAVE_CONFIG_H + #include +#endif + +#ifndef WOLFSSL_USER_SETTINGS + #include +#endif +#include +#include + +#ifndef WOLFPKCS11_USER_SETTINGS + #include +#endif +#include + +#include +#include + +#if !defined(WOLFPKCS11_NO_STORE) && !defined(WOLFPKCS11_CUSTOM_STORE) && \ + !defined(WOLFPKCS11_TPM_STORE) && !defined(WOLFPKCS11_NO_ENV) && \ + !defined(NO_AES) && !defined(_WIN32) +#define RUN_TEST +#endif + +#ifdef RUN_TEST + +#include +#include +#include +#include + +#ifndef HAVE_PKCS11_STATIC +#include +#endif + +#include "testdata.h" + +#define STORE_DIR "./store/partial" +#define SNAP_DIR "./store/partial-snap" + +#define MAX_FILES 64 +#define NAME_LEN 128 + +#define CHECK(cond, msg) \ + do { \ + if (!(cond)) { \ + fprintf(stderr, "%s:%d - %s - FAIL\n", __FILE__, __LINE__, \ + msg); \ + return -1; \ + } \ + } \ + while (0) +#define CHECK_RV(rv, msg) \ + do { \ + if ((rv) != CKR_OK) { \ + fprintf(stderr, "%s:%d - %s: 0x%lx - FAIL\n", __FILE__, \ + __LINE__, msg, (unsigned long)(rv)); \ + return -1; \ + } \ + } \ + while (0) + +#ifndef HAVE_PKCS11_STATIC +static void* dlib; +#endif +static CK_FUNCTION_LIST* funcList; +static CK_SLOT_ID slot; +static unsigned char tokenName[32] = "wolfpkcs11"; +static byte* soPin = (byte*)"password123456"; +static int soPinLen = 14; +static byte* userPin = (byte*)"wolfpkcs11-test"; +static int userPinLen = 15; + +static CK_BBOOL ckTrue = CK_TRUE; +static CK_BBOOL ckFalse = CK_FALSE; + +static const byte aesKey[32] = { + 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, + 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, + 0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef, + 0xfe, 0xdc, 0xba, 0x98, 0x76, 0x54, 0x32, 0x10 +}; + +/* Objects made by the test. Index is used in the label and value. */ +#define OBJ_CNT 7 +#define OBJ_AES 5 + +struct file_ent { + char name[NAME_LEN]; + ino_t ino; +}; + +/* ---- Store directory helpers ---- */ + +static int is_object_record(const char* name) +{ + /* Object records are named "wp11___". */ + const char* p = strrchr(name, '_'); + + if (strncmp(name, "wp11_", 5) != 0 || strncmp(name, "wp11_token_", 11) == 0) + return 0; + return p != NULL && strlen(p + 1) == 16 && strchr(name, '.') == NULL; +} + +static long record_id(const char* name) +{ + return strtol(strrchr(name, '_') + 1, NULL, 16); +} + +static int list_records(const char* dir, struct file_ent* ents, int* cnt) +{ + DIR* d; + struct dirent* e; + struct stat st; + char path[NAME_LEN * 2]; + + *cnt = 0; + d = opendir(dir); + if (d == NULL) + return -1; + while ((e = readdir(d)) != NULL) { + if (!is_object_record(e->d_name) || *cnt >= MAX_FILES) + continue; + snprintf(path, sizeof(path), "%s/%s", dir, e->d_name); + if (stat(path, &st) != 0) + continue; + snprintf(ents[*cnt].name, NAME_LEN, "%s", e->d_name); + ents[*cnt].ino = st.st_ino; + (*cnt)++; + } + closedir(d); + return 0; +} + +static int clear_dir(const char* dir) +{ + DIR* d; + struct dirent* e; + char path[NAME_LEN * 2]; + + if (mkdir(dir, 0700) != 0 && errno != EEXIST) + return -1; + d = opendir(dir); + if (d == NULL) + return -1; + while ((e = readdir(d)) != NULL) { + if (e->d_name[0] == '.') + continue; + snprintf(path, sizeof(path), "%s/%s", dir, e->d_name); + (void)unlink(path); + } + closedir(d); + return 0; +} + +static int copy_dir(const char* from, const char* to) +{ + DIR* d; + struct dirent* e; + char src[NAME_LEN * 2]; + char dst[NAME_LEN * 2]; + char buf[4096]; + size_t n; + FILE* in; + FILE* out; + + if (clear_dir(to) != 0) + return -1; + d = opendir(from); + if (d == NULL) + return -1; + while ((e = readdir(d)) != NULL) { + if (e->d_name[0] == '.') + continue; + snprintf(src, sizeof(src), "%s/%s", from, e->d_name); + snprintf(dst, sizeof(dst), "%s/%s", to, e->d_name); + in = fopen(src, "rb"); + out = fopen(dst, "wb"); + if (in == NULL || out == NULL) { + if (in != NULL) + fclose(in); + if (out != NULL) + fclose(out); + closedir(d); + return -1; + } + while ((n = fread(buf, 1, sizeof(buf), in)) > 0) + (void)fwrite(buf, 1, n, out); + fclose(in); + fclose(out); + } + closedir(d); + return 0; +} + +/* ---- PKCS#11 helpers ---- */ + +static CK_RV pkcs11_init(void) +{ + CK_RV rv; + CK_C_INITIALIZE_ARGS args; + CK_SLOT_ID slotList[16]; + CK_ULONG slotCount = sizeof(slotList) / sizeof(slotList[0]); +#ifndef HAVE_PKCS11_STATIC + CK_C_GetFunctionList func; + + dlib = dlopen(WOLFPKCS11_DLL_FILENAME, RTLD_NOW | RTLD_LOCAL); + if (dlib == NULL) { + fprintf(stderr, "dlopen error: %s\n", dlerror()); + return CKR_GENERAL_ERROR; + } + func = (CK_C_GetFunctionList)dlsym(dlib, "C_GetFunctionList"); + if (func == NULL) + return CKR_GENERAL_ERROR; + rv = func(&funcList); +#else + rv = C_GetFunctionList(&funcList); +#endif + if (rv != CKR_OK) + return rv; + + XMEMSET(&args, 0, sizeof(args)); + args.flags = CKF_OS_LOCKING_OK; + rv = funcList->C_Initialize(&args); + if (rv == CKR_OK) + rv = funcList->C_GetSlotList(CK_TRUE, slotList, &slotCount); + if (rv == CKR_OK && slotCount == 0) + rv = CKR_GENERAL_ERROR; + if (rv == CKR_OK) + slot = slotList[0]; + return rv; +} + +static void pkcs11_final(void) +{ + (void)funcList->C_Finalize(NULL); +#ifndef HAVE_PKCS11_STATIC + if (dlib != NULL) { + dlclose(dlib); + dlib = NULL; + } +#endif +} + +static CK_RV open_user_session(CK_SESSION_HANDLE* session) +{ + CK_RV rv; + + rv = funcList->C_OpenSession(slot, CKF_SERIAL_SESSION | CKF_RW_SESSION, + NULL, NULL, session); + if (rv == CKR_OK) + rv = funcList->C_Login(*session, CKU_USER, userPin, userPinLen); + return rv; +} + +static CK_RV provision(void) +{ + CK_RV rv; + CK_SESSION_HANDLE session; + + rv = funcList->C_InitToken(slot, soPin, soPinLen, tokenName); + if (rv == CKR_OK) { + rv = funcList->C_OpenSession(slot, + CKF_SERIAL_SESSION | CKF_RW_SESSION, NULL, NULL, &session); + } + if (rv == CKR_OK) { + rv = funcList->C_Login(session, CKU_SO, soPin, soPinLen); + if (rv == CKR_OK) + rv = funcList->C_InitPIN(session, userPin, userPinLen); + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + } + return rv; +} + +static void obj_label(int idx, char* label, size_t len) +{ + snprintf(label, len, "obj%d", idx); +} + +static CK_RV create_obj(CK_SESSION_HANDLE session, int idx, + CK_OBJECT_HANDLE* obj) +{ + char label[16]; + char value[32]; + CK_OBJECT_CLASS dataClass = CKO_DATA; + CK_OBJECT_CLASS keyClass = CKO_SECRET_KEY; + CK_KEY_TYPE keyType = CKK_AES; + + obj_label(idx, label, sizeof(label)); + snprintf(value, sizeof(value), "value of object %d", idx); + if (idx == OBJ_AES) { + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &keyClass, sizeof(keyClass) }, + { CKA_KEY_TYPE, &keyType, sizeof(keyType) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckTrue, sizeof(ckTrue) }, + { CKA_SENSITIVE, &ckFalse, sizeof(ckFalse) }, + { CKA_EXTRACTABLE, &ckTrue, sizeof(ckTrue) }, + { CKA_ENCRYPT, &ckTrue, sizeof(ckTrue) }, + { CKA_LABEL, label, strlen(label) }, + { CKA_VALUE, (void*)aesKey, sizeof(aesKey) }, + }; + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(tmpl[0]), obj); + } + else { + CK_ATTRIBUTE tmpl[] = { + { CKA_CLASS, &dataClass, sizeof(dataClass) }, + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_PRIVATE, &ckFalse, sizeof(ckFalse) }, + { CKA_MODIFIABLE, &ckTrue, sizeof(ckTrue) }, + { CKA_LABEL, label, strlen(label) }, + { CKA_VALUE, value, strlen(value) }, + }; + return funcList->C_CreateObject(session, tmpl, + sizeof(tmpl) / sizeof(tmpl[0]), obj); + } +} + +/* Find the object with the given label. Returns the number found. */ +static int find_label(CK_SESSION_HANDLE session, const char* label, + CK_OBJECT_HANDLE* obj) +{ + CK_ATTRIBUTE tmpl[] = { + { CKA_TOKEN, &ckTrue, sizeof(ckTrue) }, + { CKA_LABEL, (void*)label, strlen(label) }, + }; + CK_OBJECT_HANDLE found[4]; + CK_ULONG cnt = 0; + + if (funcList->C_FindObjectsInit(session, tmpl, 2) != CKR_OK) + return -1; + if (funcList->C_FindObjects(session, found, 4, &cnt) != CKR_OK) + cnt = 0; + (void)funcList->C_FindObjectsFinal(session); + if (cnt > 0) + *obj = found[0]; + return (int)cnt; +} + +static int count_token_objects(CK_SESSION_HANDLE session) +{ + CK_ATTRIBUTE tmpl[] = { { CKA_TOKEN, &ckTrue, sizeof(ckTrue) } }; + CK_OBJECT_HANDLE found[32]; + CK_ULONG cnt = 0; + + if (funcList->C_FindObjectsInit(session, tmpl, 1) != CKR_OK) + return -1; + if (funcList->C_FindObjects(session, found, 32, &cnt) != CKR_OK) + cnt = 0; + (void)funcList->C_FindObjectsFinal(session); + return (int)cnt; +} + +static int check_value(CK_SESSION_HANDLE session, CK_OBJECT_HANDLE obj, + const void* exp, CK_ULONG expLen) +{ + byte buf[64]; + CK_ATTRIBUTE attr = { CKA_VALUE, buf, sizeof(buf) }; + + if (funcList->C_GetAttributeValue(session, obj, &attr, 1) != CKR_OK) + return -1; + if (attr.ulValueLen != expLen || memcmp(buf, exp, expLen) != 0) + return -1; + return 0; +} + +/* Create an object and check no other object record was rewritten. */ +static int create_checked(CK_SESSION_HANDLE session, int idx, + CK_OBJECT_HANDLE* obj) +{ + struct file_ent before[MAX_FILES]; + struct file_ent after[MAX_FILES]; + int beforeCnt, afterCnt; + int i, j; + + CHECK(list_records(STORE_DIR, before, &beforeCnt) == 0, "List records"); + CHECK_RV(create_obj(session, idx, obj), "Create object"); + CHECK(list_records(STORE_DIR, after, &afterCnt) == 0, "List records"); + CHECK(afterCnt > beforeCnt, "Records written for new object"); + for (i = 0; i < beforeCnt; i++) { + for (j = 0; j < afterCnt; j++) { + if (strcmp(before[i].name, after[j].name) == 0) + break; + } + CHECK(j < afterCnt, "Record of other object still present"); + CHECK(before[i].ino == after[j].ino, + "Record of other object not rewritten"); + } + return 0; +} + +static int partial_store_test(void) +{ + CK_SESSION_HANDLE session; + CK_OBJECT_HANDLE obj[OBJ_CNT]; + CK_OBJECT_HANDLE h; + struct file_ent recs[MAX_FILES]; + int recCnt; + char label[16]; + char value[32]; + static const char renamed[] = "obj2-renamed"; + CK_ATTRIBUTE labelAttr = { CKA_LABEL, (void*)renamed, + sizeof(renamed) - 1 }; + int i; + int survivors[] = { 1, 4, OBJ_AES }; + + (void)mkdir("./store", 0700); + CHECK(clear_dir(STORE_DIR) == 0, "Clear store directory"); + CHECK_RV(pkcs11_init(), "Initialize"); + CHECK_RV(provision(), "Provision token"); + CHECK_RV(open_user_session(&session), "Open user session"); + + /* Objects 0 to 5, with the AES key (5) newest. */ + for (i = 0; i <= OBJ_AES; i++) + CHECK(create_checked(session, i, &obj[i]) == 0, "Create object"); + + /* Not the newest object: the newest (the AES key) takes its place. */ + CHECK_RV(funcList->C_DestroyObject(session, obj[0]), "Destroy oldest"); + /* Object 4 is now the newest and moves into the place of object 3. */ + CHECK_RV(funcList->C_DestroyObject(session, obj[3]), "Destroy middle"); + /* Newest object: nothing moves. */ + CHECK(create_checked(session, 6, &obj[6]) == 0, "Create object"); + CHECK_RV(funcList->C_DestroyObject(session, obj[6]), "Destroy newest"); + + CHECK_RV(funcList->C_SetAttributeValue(session, obj[2], &labelAttr, 1), + "Rename object"); + + /* Keep what is in storage now, as if power was lost here. */ + CHECK(copy_dir(STORE_DIR, SNAP_DIR) == 0, "Snapshot store"); + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + pkcs11_final(); + CHECK(copy_dir(SNAP_DIR, STORE_DIR) == 0, "Restore store"); + + /* Only records of the four objects left may exist. */ + CHECK(list_records(STORE_DIR, recs, &recCnt) == 0, "List records"); + for (i = 0; i < recCnt; i++) { + if (record_id(recs[i].name) >= 4) { + fprintf(stderr, "Record left behind: %s\n", recs[i].name); + CHECK(0, "No records for removed ids"); + } + } + + CHECK_RV(pkcs11_init(), "Initialize after restore"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(count_token_objects(session) == 4, "Four objects loaded"); + + for (i = 0; i < (int)(sizeof(survivors) / sizeof(survivors[0])); i++) { + obj_label(survivors[i], label, sizeof(label)); + CHECK(find_label(session, label, &h) == 1, "Object loaded"); + if (survivors[i] == OBJ_AES) { + CHECK(check_value(session, h, aesKey, sizeof(aesKey)) == 0, + "Key value loaded"); + } + else { + snprintf(value, sizeof(value), "value of object %d", + survivors[i]); + CHECK(check_value(session, h, value, strlen(value)) == 0, + "Object value loaded"); + } + } + CHECK(find_label(session, renamed, &h) == 1, "Renamed object loaded"); + snprintf(value, sizeof(value), "value of object %d", 2); + CHECK(check_value(session, h, value, strlen(value)) == 0, + "Renamed object value loaded"); + CHECK(find_label(session, "obj2", &h) == 0, "Old label gone"); + for (i = 0; i < OBJ_CNT; i++) { + if (i == 0 || i == 3 || i == 6) { + obj_label(i, label, sizeof(label)); + CHECK(find_label(session, label, &h) == 0, + "Destroyed object not loaded"); + } + } + + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + pkcs11_final(); + return 0; +} + +#endif /* RUN_TEST */ + +int main(int argc, char* argv[]) +{ +#ifdef RUN_TEST + int ret; + + (void)argc; + (void)argv; + /* The test empties the store directory, so always use its own. */ + XSETENV("WOLFPKCS11_TOKEN_PATH", STORE_DIR, 1); + + printf("Token partial store test\n"); + ret = partial_store_test(); + if (ret == 0) + printf("All tests passed!\n"); + return (ret == 0) ? 0 : 1; +#else + (void)argc; + (void)argv; + printf("Test not built for this configuration\n"); + return 77; +#endif +} diff --git a/wolfpkcs11/internal.h b/wolfpkcs11/internal.h index 41c2e763..405202d0 100644 --- a/wolfpkcs11/internal.h +++ b/wolfpkcs11/internal.h @@ -559,6 +559,7 @@ WP11_LOCAL int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest); WP11_LOCAL CK_OBJECT_HANDLE WP11_Object_GetHandle(WP11_Object* object); WP11_LOCAL int WP11_Object_OnToken(WP11_Object* object); +WP11_LOCAL int WP11_Object_Persist(WP11_Object* object); WP11_LOCAL CK_KEY_TYPE WP11_Object_GetType(WP11_Object* object); WP11_LOCAL CK_ULONG WP11_Object_GetDevId(WP11_Object* object); From e46f06b33a89e7d9ef793ff41dc9424584f10748 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Fri, 16 Oct 2026 12:59:21 +0100 Subject: [PATCH 2/3] Catch up storage after a skipped or failed token store With only changed objects written, a store pass that was skipped, because WOLFPKCS11_NO_STORE was set, or that failed was no longer repaired by the next create or destroy rewriting everything. Mark the token when that happens so its next store pass writes all objects. C_SetAttributeValue no longer fails when its write fails. The change is already live, and the marked token has it written by the next store pass or C_Finalize, as happened before attribute changes were written here. Also fix the test build with GCC format truncation checks, and test changes made while storage is turned off. --- src/crypto.c | 4 +- src/internal.c | 56 +++++++---- tests/token_partial_store_test.c | 159 +++++++++++++++++++++++-------- wolfpkcs11/internal.h | 2 +- 4 files changed, 161 insertions(+), 60 deletions(-) diff --git a/src/crypto.c b/src/crypto.c index b4537bad..27b664e0 100644 --- a/src/crypto.c +++ b/src/crypto.c @@ -2558,8 +2558,8 @@ static CK_RV wp11_C_SetAttributeValue(CK_SESSION_HANDLE hSession, if (rv == CKR_OK) rv = SetAttributeValue(session, obj, pTemplate, ulCount, CK_FALSE); /* Token objects are only rewritten when they change, so write it now. */ - if (rv == CKR_OK && WP11_Object_Persist(obj) != 0) - rv = CKR_FUNCTION_FAILED; + if (rv == CKR_OK) + WP11_Object_Persist(obj); WOLFPKCS11_LEAVE("C_SetAttributeValue", rv); return rv; } diff --git a/src/internal.c b/src/internal.c index 02afafde..d3f56a64 100644 --- a/src/internal.c +++ b/src/internal.c @@ -567,6 +567,12 @@ typedef struct WP11_Token { * 0 = not set * 1 = empty * 2 = not empty */ +#ifndef WOLFPKCS11_NO_STORE + byte storeDirty; /* A store pass was skipped or failed, + * so the next one writes everything. + * Not a bit-field: guarded by the + * token lock, unlike userPinEmpty. */ +#endif } WP11_Token; struct WP11_ObjectCalls { @@ -9100,7 +9106,8 @@ static int wp11_Token_Load(WP11_Slot* slot, int tokenId, WP11_Token* token) * @return 0 on success. * @return MEMORY_E when dynamic memory allocation fails. * @return BUFFER_E when storing fails. - * @return NOT_AVAILABLE_E when unable to write data. + * @return NOT_AVAILABLE_E when storage is turned off and nothing was + * written. */ static int wp11_Token_StorePass(WP11_Token* token, int tokenId, int what, WP11_Object* only) @@ -9149,7 +9156,7 @@ static int wp11_Token_StorePass(WP11_Token* token, int tokenId, int what, if (what == WP11_STORE_PASS_OBJECT) { /* Not writing the token record. */ - return (ret == NOT_AVAILABLE_E) ? 0 : ret; + return ret; } if (ret == 0) { @@ -9235,10 +9242,6 @@ static int wp11_Token_StorePass(WP11_Token* token, int tokenId, int what, ret = wp11_storage_close_write(storage, ret); } - else if (ret == NOT_AVAILABLE_E) { - /* Not writing. */ - ret = 0; - } return ret; } @@ -9246,11 +9249,14 @@ static int wp11_Token_StorePass(WP11_Token* token, int tokenId, int what, /** * Store records of the token as one pass that commits as a unit. * + * When an earlier pass was skipped, because storage was turned off, or failed, + * storage may be behind the token in memory, so this pass writes everything. + * * @param [in] token Token object. * @param [in] tokenId Id of token. * @param [in] what WP11_STORE_PASS_* value. * @param [in] only Object to store when not storing all of them. - * @return 0 on success. + * @return 0 on success, or when storage is turned off. * @return Other value when the pass did not commit. */ static int wp11_Token_StoreSome(WP11_Token* token, int tokenId, int what, @@ -9259,11 +9265,16 @@ static int wp11_Token_StoreSome(WP11_Token* token, int tokenId, int what, int ret; #ifdef WP11_STORE_BATCH int endRet; +#endif + + if (token->storeDirty) + what = WP11_STORE_PASS_ALL; +#ifdef WP11_STORE_BATCH ret = wolfPKCS11_StoreBatchBegin(tokenId); if (ret == 0) { ret = wp11_Token_StorePass(token, tokenId, what, only); - endRet = wolfPKCS11_StoreBatchEnd(ret == 0); + endRet = wolfPKCS11_StoreBatchEnd(ret == 0 || ret == NOT_AVAILABLE_E); if (ret == 0) ret = endRet; } @@ -9271,6 +9282,17 @@ static int wp11_Token_StoreSome(WP11_Token* token, int tokenId, int what, ret = wp11_Token_StorePass(token, tokenId, what, only); #endif + if (ret == 0) { + if (what == WP11_STORE_PASS_ALL) + token->storeDirty = 0; + } + else { + token->storeDirty = 1; + /* Storage is turned off: nothing to write is not an error. */ + if (ret == NOT_AVAILABLE_E) + ret = 0; + } + return ret; } @@ -12930,9 +12952,10 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, /* A token record left unwritten outranks a record left behind. */ if (storeRet != 0) ret = storeRet; - if (moved != NULL && storeRet == 0) { + if (moved != NULL && storeRet == 0 && !token->storeDirty) { /* Remove the moved object's records under its old id, the - * highest one, which is no longer used. */ + * highest one, which is no longer used. Kept when the moved + * object was not written under its new id. */ storeRet = wp11_Object_Unstore(moved, (int)session->slotId, token->objCnt); #ifdef DEBUG_WOLFPKCS11 @@ -13534,21 +13557,20 @@ int WP11_Object_OnToken(WP11_Object* object) /** * Write a token object to storage after its attributes have changed. * Only the object's own records are written. Session objects are not stored. + * The change is already live, so when the write fails the token is left + * marked for a full store by its next store pass or C_Finalize. * * @param object [in] Object object. - * @return 0 on success or when nothing needs storing. - * Other value when storing failed. */ -int WP11_Object_Persist(WP11_Object* object) +void WP11_Object_Persist(WP11_Object* object) { - int ret = 0; #ifndef WOLFPKCS11_NO_STORE WP11_Slot* slot = object->slot; WP11_Token* token; WP11_Object* curr; if (!object->onToken || slot == NULL) - return 0; + return; token = &slot->token; WP11_Lock_LockRW(&token->lock); @@ -13558,15 +13580,13 @@ int WP11_Object_Persist(WP11_Object* object) break; } if (curr != NULL) { - ret = wp11_Slot_StoreSome(slot, (int)slot->id, WP11_STORE_PASS_OBJECT, + (void)wp11_Slot_StoreSome(slot, (int)slot->id, WP11_STORE_PASS_OBJECT, object); } WP11_Lock_UnlockRW(&token->lock); #else (void)object; #endif - - return ret; } /** diff --git a/tests/token_partial_store_test.c b/tests/token_partial_store_test.c index d507d7c4..469610bc 100644 --- a/tests/token_partial_store_test.c +++ b/tests/token_partial_store_test.c @@ -24,7 +24,9 @@ * - after creates, destroys and an attribute change, the records written so * far load back as the right objects without relying on the full store * done by C_Finalize, - * - no records are left behind for destroyed objects. + * - no records are left behind for destroyed objects, + * - changes made while storage is turned off (WOLFPKCS11_NO_STORE) are all + * written once it is turned back on. */ #ifdef HAVE_CONFIG_H @@ -69,6 +71,7 @@ #define MAX_FILES 64 #define NAME_LEN 128 +#define PATH_LEN 256 #define CHECK(cond, msg) \ do { \ @@ -136,24 +139,35 @@ static long record_id(const char* name) return strtol(strrchr(name, '_') + 1, NULL, 16); } +/* Join a directory and file name. Returns -1 when it does not fit. */ +static int join_path(char* path, size_t len, const char* dir, const char* name) +{ + int n = snprintf(path, len, "%s/%s", dir, name); + + return (n < 0 || (size_t)n >= len) ? -1 : 0; +} + static int list_records(const char* dir, struct file_ent* ents, int* cnt) { DIR* d; struct dirent* e; struct stat st; - char path[NAME_LEN * 2]; + char path[PATH_LEN]; *cnt = 0; d = opendir(dir); if (d == NULL) return -1; while ((e = readdir(d)) != NULL) { - if (!is_object_record(e->d_name) || *cnt >= MAX_FILES) + if (!is_object_record(e->d_name) || *cnt >= MAX_FILES || + strlen(e->d_name) >= NAME_LEN) { continue; - snprintf(path, sizeof(path), "%s/%s", dir, e->d_name); - if (stat(path, &st) != 0) + } + if (join_path(path, sizeof(path), dir, e->d_name) != 0 || + stat(path, &st) != 0) { continue; - snprintf(ents[*cnt].name, NAME_LEN, "%s", e->d_name); + } + memcpy(ents[*cnt].name, e->d_name, strlen(e->d_name) + 1); ents[*cnt].ino = st.st_ino; (*cnt)++; } @@ -165,7 +179,7 @@ static int clear_dir(const char* dir) { DIR* d; struct dirent* e; - char path[NAME_LEN * 2]; + char path[PATH_LEN]; if (mkdir(dir, 0700) != 0 && errno != EEXIST) return -1; @@ -175,8 +189,8 @@ static int clear_dir(const char* dir) while ((e = readdir(d)) != NULL) { if (e->d_name[0] == '.') continue; - snprintf(path, sizeof(path), "%s/%s", dir, e->d_name); - (void)unlink(path); + if (join_path(path, sizeof(path), dir, e->d_name) == 0) + (void)unlink(path); } closedir(d); return 0; @@ -186,8 +200,8 @@ static int copy_dir(const char* from, const char* to) { DIR* d; struct dirent* e; - char src[NAME_LEN * 2]; - char dst[NAME_LEN * 2]; + char src[PATH_LEN]; + char dst[PATH_LEN]; char buf[4096]; size_t n; FILE* in; @@ -201,8 +215,11 @@ static int copy_dir(const char* from, const char* to) while ((e = readdir(d)) != NULL) { if (e->d_name[0] == '.') continue; - snprintf(src, sizeof(src), "%s/%s", from, e->d_name); - snprintf(dst, sizeof(dst), "%s/%s", to, e->d_name); + if (join_path(src, sizeof(src), from, e->d_name) != 0 || + join_path(dst, sizeof(dst), to, e->d_name) != 0) { + closedir(d); + return -1; + } in = fopen(src, "rb"); out = fopen(dst, "wb"); if (in == NULL || out == NULL) { @@ -420,15 +437,50 @@ static int create_checked(CK_SESSION_HANDLE session, int idx, return 0; } +/* Reload from what is in storage now, as if power was lost here, and check + * only records for ids below cnt exist. */ +static int reload_from_storage(CK_SESSION_HANDLE session, int cnt) +{ + struct file_ent recs[MAX_FILES]; + int recCnt; + int i; + + CHECK(copy_dir(STORE_DIR, SNAP_DIR) == 0, "Snapshot store"); + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + pkcs11_final(); + CHECK(copy_dir(SNAP_DIR, STORE_DIR) == 0, "Restore store"); + + CHECK(list_records(STORE_DIR, recs, &recCnt) == 0, "List records"); + for (i = 0; i < recCnt; i++) { + if (record_id(recs[i].name) >= cnt) { + fprintf(stderr, "Record left behind: %s\n", recs[i].name); + CHECK(0, "No records for removed ids"); + } + } + CHECK_RV(pkcs11_init(), "Initialize after restore"); + return 0; +} + +static int check_data_obj(CK_SESSION_HANDLE session, const char* label, + int idx) +{ + CK_OBJECT_HANDLE h; + char value[32]; + + CHECK(find_label(session, label, &h) == 1, "Object loaded"); + snprintf(value, sizeof(value), "value of object %d", idx); + CHECK(check_value(session, h, value, strlen(value)) == 0, + "Object value loaded"); + return 0; +} + static int partial_store_test(void) { CK_SESSION_HANDLE session; CK_OBJECT_HANDLE obj[OBJ_CNT]; CK_OBJECT_HANDLE h; - struct file_ent recs[MAX_FILES]; - int recCnt; char label[16]; - char value[32]; static const char renamed[] = "obj2-renamed"; CK_ATTRIBUTE labelAttr = { CKA_LABEL, (void*)renamed, sizeof(renamed) - 1 }; @@ -456,23 +508,8 @@ static int partial_store_test(void) CHECK_RV(funcList->C_SetAttributeValue(session, obj[2], &labelAttr, 1), "Rename object"); - /* Keep what is in storage now, as if power was lost here. */ - CHECK(copy_dir(STORE_DIR, SNAP_DIR) == 0, "Snapshot store"); - (void)funcList->C_Logout(session); - (void)funcList->C_CloseSession(session); - pkcs11_final(); - CHECK(copy_dir(SNAP_DIR, STORE_DIR) == 0, "Restore store"); - /* Only records of the four objects left may exist. */ - CHECK(list_records(STORE_DIR, recs, &recCnt) == 0, "List records"); - for (i = 0; i < recCnt; i++) { - if (record_id(recs[i].name) >= 4) { - fprintf(stderr, "Record left behind: %s\n", recs[i].name); - CHECK(0, "No records for removed ids"); - } - } - - CHECK_RV(pkcs11_init(), "Initialize after restore"); + CHECK(reload_from_storage(session, 4) == 0, "Reload"); CHECK_RV(open_user_session(&session), "Open user session"); CHECK(count_token_objects(session) == 4, "Four objects loaded"); @@ -484,16 +521,11 @@ static int partial_store_test(void) "Key value loaded"); } else { - snprintf(value, sizeof(value), "value of object %d", - survivors[i]); - CHECK(check_value(session, h, value, strlen(value)) == 0, + CHECK(check_data_obj(session, label, survivors[i]) == 0, "Object value loaded"); } } - CHECK(find_label(session, renamed, &h) == 1, "Renamed object loaded"); - snprintf(value, sizeof(value), "value of object %d", 2); - CHECK(check_value(session, h, value, strlen(value)) == 0, - "Renamed object value loaded"); + CHECK(check_data_obj(session, renamed, 2) == 0, "Renamed object loaded"); CHECK(find_label(session, "obj2", &h) == 0, "Old label gone"); for (i = 0; i < OBJ_CNT; i++) { if (i == 0 || i == 3 || i == 6) { @@ -509,6 +541,51 @@ static int partial_store_test(void) return 0; } +static int pause_resume_test(void) +{ + CK_SESSION_HANDLE session; + CK_OBJECT_HANDLE obj[5]; + CK_OBJECT_HANDLE h; + static const char renamed[] = "obj1-renamed"; + CK_ATTRIBUTE labelAttr = { CKA_LABEL, (void*)renamed, + sizeof(renamed) - 1 }; + int i; + + CHECK(clear_dir(STORE_DIR) == 0, "Clear store directory"); + CHECK_RV(pkcs11_init(), "Initialize"); + CHECK_RV(provision(), "Provision token"); + CHECK_RV(open_user_session(&session), "Open user session"); + for (i = 0; i <= 2; i++) + CHECK_RV(create_obj(session, i, &obj[i]), "Create object"); + + /* Storage off: these changes are only in memory. */ + CHECK(setenv("WOLFPKCS11_NO_STORE", "1", 1) == 0, "Turn storage off"); + CHECK_RV(funcList->C_DestroyObject(session, obj[0]), "Destroy oldest"); + CHECK_RV(funcList->C_SetAttributeValue(session, obj[1], &labelAttr, 1), + "Rename object"); + CHECK(find_label(session, renamed, &h) == 1, "Rename is live"); + CHECK_RV(create_obj(session, 3, &obj[3]), "Create object"); + CHECK(unsetenv("WOLFPKCS11_NO_STORE") == 0, "Turn storage on"); + + /* The next write catches up with every change made while off. */ + CHECK_RV(create_obj(session, 4, &obj[4]), "Create object"); + + CHECK(reload_from_storage(session, 4) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(count_token_objects(session) == 4, "Four objects loaded"); + CHECK(check_data_obj(session, renamed, 1) == 0, "Renamed object"); + CHECK(check_data_obj(session, "obj2", 2) == 0, "Moved object"); + CHECK(check_data_obj(session, "obj3", 3) == 0, "Object made while off"); + CHECK(check_data_obj(session, "obj4", 4) == 0, "Object made after"); + CHECK(find_label(session, "obj0", &h) == 0, "Destroyed object gone"); + CHECK(find_label(session, "obj1", &h) == 0, "Old label gone"); + + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + pkcs11_final(); + return 0; +} + #endif /* RUN_TEST */ int main(int argc, char* argv[]) @@ -523,6 +600,10 @@ int main(int argc, char* argv[]) printf("Token partial store test\n"); ret = partial_store_test(); + if (ret == 0) { + printf("Token pause and resume test\n"); + ret = pause_resume_test(); + } if (ret == 0) printf("All tests passed!\n"); return (ret == 0) ? 0 : 1; diff --git a/wolfpkcs11/internal.h b/wolfpkcs11/internal.h index 405202d0..06b54ec2 100644 --- a/wolfpkcs11/internal.h +++ b/wolfpkcs11/internal.h @@ -559,7 +559,7 @@ WP11_LOCAL int WP11_Object_Copy(WP11_Object *src, WP11_Object *dest); WP11_LOCAL CK_OBJECT_HANDLE WP11_Object_GetHandle(WP11_Object* object); WP11_LOCAL int WP11_Object_OnToken(WP11_Object* object); -WP11_LOCAL int WP11_Object_Persist(WP11_Object* object); +WP11_LOCAL void WP11_Object_Persist(WP11_Object* object); WP11_LOCAL CK_KEY_TYPE WP11_Object_GetType(WP11_Object* object); WP11_LOCAL CK_ULONG WP11_Object_GetDevId(WP11_Object* object); From 9de45bd92a980c75177d912a9e8973102f106c9d Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Mon, 19 Oct 2026 11:00:49 +0100 Subject: [PATCH 3/3] Remove destroyed object records after the store pass A destroy removed the object's records before writing the token, so a pass that failed left the token record pointing at a missing id and the next C_Initialize failed. Write the moved object and the token record first, and only then remove what is no longer used: the destroyed head's records, or the moved object's records under its old id and the destroyed object's key record when its store type differs from the moved object's. Also log a failed write of a changed object in debug builds, update the slot store doc blocks, and test destroys and attribute changes whose store fails. --- src/internal.c | 140 ++++++++++++++++------- tests/token_partial_store_test.c | 190 ++++++++++++++++++++++++++++++- 2 files changed, 282 insertions(+), 48 deletions(-) diff --git a/src/internal.c b/src/internal.c index d3f56a64..e2d27f0f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8564,29 +8564,16 @@ static int wp11_Token_EncodeObjects(WP11_Token* token, int protect) } /** - * Unstore a key object to storage. - * - * Empties the contents of the object. + * Get the store type of the record holding an object's key or value data, + * which is written alongside its WOLFPKCS11_STORE_OBJECT record. * - * @param [in] object Key object. - * @param [in] tokenId Id of token this key belongs to. - * @param [in] objId Id of object for token. + * @param [in] object Object object. + * @return WOLFPKCS11_STORE_* value, or -1 when the object has no such record. */ -static int wp11_Object_Unstore(WP11_Object* object, int tokenId, int objId) +static int wp11_Object_StoreType(WP11_Object* object) { - int ret; int storeObjType = -1; - if (objId < 0) { - return BAD_FUNC_ARG; - } - - /* Remove store object */ - ret = wp11_storage_remove(WOLFPKCS11_STORE_OBJECT, tokenId, objId); - if (ret != 0) { - return ret; - } - /* CKK_* and CKC_* values overlap, check for cert separately */ if (object->objClass == CKO_CERTIFICATE) { storeObjType = WOLFPKCS11_STORE_CERT; @@ -8664,8 +8651,35 @@ static int wp11_Object_Unstore(WP11_Object* object, int tokenId, int objId) break; } } + + return storeObjType; +} + +/** + * Unstore a key object to storage. + * + * Empties the contents of the object. + * + * @param [in] object Key object. + * @param [in] tokenId Id of token this key belongs to. + * @param [in] objId Id of object for token. + */ +static int wp11_Object_Unstore(WP11_Object* object, int tokenId, int objId) +{ + int ret; + + if (objId < 0) { + return BAD_FUNC_ARG; + } + + /* Remove store object */ + ret = wp11_storage_remove(WOLFPKCS11_STORE_OBJECT, tokenId, objId); + if (ret != 0) { + return ret; + } + /* remove actual object by type */ - return wp11_storage_remove(storeObjType, tokenId, objId); + return wp11_storage_remove(wp11_Object_StoreType(object), tokenId, objId); } #endif /* !WOLFPKCS11_NO_STORE */ @@ -9519,14 +9533,17 @@ static int wp11_Slot_Load(WP11_Slot* slot, int id) } /** - * Store tokens in slot to storage. + * Store records of the slot's token, once the token has been provisioned. * - * @param [in] object Slot object. - * @param [in] slotId Id of slot this key belongs to. - * @return 0 on success. + * @param [in] slot Slot object. + * @param [in] id Id of the slot's token in storage. + * @param [in] what WP11_STORE_PASS_* value. + * @param [in] only Object to store when not storing all of them. + * @return 0 on success, when storage is turned off, or when the token is not + * provisioned. * @return MEMORY_E when dynamic memory allocation fails. * @return BUFFER_E when storing fails. - * @return NOT_AVAILABLE_E when unable to write data. + * @return Other value when the pass did not commit. */ static int wp11_Slot_StoreSome(WP11_Slot* slot, int id, int what, WP11_Object* only) @@ -9540,6 +9557,16 @@ static int wp11_Slot_StoreSome(WP11_Slot* slot, int id, int what, return wp11_Token_StoreSome(&slot->token, id, what, only); } +/** + * Store the slot's token and all its objects, once the token has been + * provisioned. + * + * @param [in] slot Slot object. + * @param [in] id Id of the slot's token in storage. + * @return 0 on success, when storage is turned off, or when the token is not + * provisioned. + * @return Other value when the pass did not commit. + */ static int wp11_Slot_Store(WP11_Slot* slot, int id) { return wp11_Slot_StoreSome(slot, id, WP11_STORE_PASS_ALL, NULL); @@ -12930,17 +12957,9 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, } #ifndef WOLFPKCS11_NO_STORE - storeRet = wp11_Object_Unstore(object, (int)session->slotId, id); - #ifdef DEBUG_WOLFPKCS11 - if (storeRet != 0) { - printf("Failed to unstore slot %d, id: %d, ret: %d\n", - (int)session->slotId, id, storeRet); - } - #endif - /* NOT_AVAILABLE_E: storage is disabled, nothing to remove. */ - if (ret == 0 && storeRet != NOT_AVAILABLE_E) - ret = storeRet; - /* Write the moved object under its new id and the token record. */ + /* Write the moved object under the freed id, and the token record, + * before removing any record, so a pass that fails leaves the + * previous records loadable. */ storeRet = wp11_Slot_StoreSome(session->slot, (int)session->slotId, WP11_STORE_PASS_TOKEN, moved); #ifdef DEBUG_WOLFPKCS11 @@ -12949,15 +12968,41 @@ int WP11_Session_RemoveObjectByHandle(WP11_Session* session, (int)session->slotId, storeRet); } #endif - /* A token record left unwritten outranks a record left behind. */ if (storeRet != 0) ret = storeRet; - if (moved != NULL && storeRet == 0 && !token->storeDirty) { - /* Remove the moved object's records under its old id, the - * highest one, which is no longer used. Kept when the moved - * object was not written under its new id. */ - storeRet = wp11_Object_Unstore(moved, (int)session->slotId, - token->objCnt); + /* Remove records only once the pass wrote everything. Otherwise the + * removed object's records stay behind, unreferenced once a later + * pass has written the token. */ + if (storeRet == 0 && !token->storeDirty) { + if (moved == NULL) { + /* The newest object went: its records, under the highest id, + * are no longer used. */ + storeRet = wp11_Object_Unstore(object, (int)session->slotId, + id); + } + else { + /* The moved object's object record replaced the removed one, + * and so did its key record when the store types match. */ + if (wp11_Object_StoreType(object) != + wp11_Object_StoreType(moved)) { + storeRet = wp11_storage_remove( + wp11_Object_StoreType(object), (int)session->slotId, + id); + #ifdef DEBUG_WOLFPKCS11 + if (storeRet != 0) { + printf("Failed to unstore slot %d, id: %d, ret: %d\n", + (int)session->slotId, id, storeRet); + } + #endif + /* NOT_AVAILABLE_E: nothing to remove. */ + if (ret == 0 && storeRet != NOT_AVAILABLE_E) + ret = storeRet; + } + /* The moved object's old id, the highest one, is no longer + * used. */ + storeRet = wp11_Object_Unstore(moved, (int)session->slotId, + token->objCnt); + } #ifdef DEBUG_WOLFPKCS11 if (storeRet != 0) { printf("Failed to unstore slot %d, id: %d, ret: %d\n", @@ -13568,6 +13613,7 @@ void WP11_Object_Persist(WP11_Object* object) WP11_Slot* slot = object->slot; WP11_Token* token; WP11_Object* curr; + int ret; if (!object->onToken || slot == NULL) return; @@ -13580,8 +13626,16 @@ void WP11_Object_Persist(WP11_Object* object) break; } if (curr != NULL) { - (void)wp11_Slot_StoreSome(slot, (int)slot->id, WP11_STORE_PASS_OBJECT, + ret = wp11_Slot_StoreSome(slot, (int)slot->id, WP11_STORE_PASS_OBJECT, object); + #ifdef DEBUG_WOLFPKCS11 + if (ret != 0) { + printf("Failed to store changed object, slot %d, ret: %d\n", + (int)slot->id, ret); + } + #else + (void)ret; + #endif } WP11_Lock_UnlockRW(&token->lock); #else diff --git a/tests/token_partial_store_test.c b/tests/token_partial_store_test.c index 469610bc..aa4ee9e9 100644 --- a/tests/token_partial_store_test.c +++ b/tests/token_partial_store_test.c @@ -26,7 +26,9 @@ * done by C_Finalize, * - no records are left behind for destroyed objects, * - changes made while storage is turned off (WOLFPKCS11_NO_STORE) are all - * written once it is turned back on. + * written once it is turned back on, + * - with a debug build, a destroy or attribute change whose store fails + * leaves the previous records loadable, and the next write catches up. */ #ifdef HAVE_CONFIG_H @@ -45,6 +47,7 @@ #include #include +#include #include #if !defined(WOLFPKCS11_NO_STORE) && !defined(WOLFPKCS11_CUSTOM_STORE) && \ @@ -206,6 +209,7 @@ static int copy_dir(const char* from, const char* to) size_t n; FILE* in; FILE* out; + int ret = 0; if (clear_dir(to) != 0) return -1; @@ -230,13 +234,20 @@ static int copy_dir(const char* from, const char* to) closedir(d); return -1; } - while ((n = fread(buf, 1, sizeof(buf), in)) > 0) - (void)fwrite(buf, 1, n, out); + while (ret == 0 && (n = fread(buf, 1, sizeof(buf), in)) > 0) { + if (fwrite(buf, 1, n, out) != n) + ret = -1; + } + if (ferror(in)) + ret = -1; fclose(in); - fclose(out); + if (fclose(out) != 0) + ret = -1; + if (ret != 0) + break; } closedir(d); - return 0; + return ret; } /* ---- PKCS#11 helpers ---- */ @@ -586,6 +597,169 @@ static int pause_resume_test(void) return 0; } +#ifdef DEBUG_WOLFPKCS11 +#ifdef __cplusplus +extern "C" { +#endif +/* Debug-only hook exported by libwolfpkcs11. */ +extern void WP11_Test_StoreCloseFailAfter(int closes); +#ifdef __cplusplus +} +#endif + +static int check_key(CK_SESSION_HANDLE session, CK_BBOOL encrypt) +{ + CK_OBJECT_HANDLE h; + CK_BBOOL val = CK_FALSE; + CK_ATTRIBUTE attr = { CKA_ENCRYPT, &val, sizeof(val) }; + + CHECK(find_label(session, "obj5", &h) == 1, "Key loaded"); + CHECK_RV(funcList->C_GetAttributeValue(session, h, &attr, 1), + "Get CKA_ENCRYPT"); + CHECK(val == encrypt, "CKA_ENCRYPT loaded"); + CHECK(check_value(session, h, aesKey, sizeof(aesKey)) == 0, + "Key value loaded"); + return 0; +} + +/* Change the key's CKA_ENCRYPT, which binds its stored value to the new + * policy, with the store failing when fail is set. */ +static int set_key_encrypt(CK_SESSION_HANDLE session, CK_BBOOL encrypt, + int fail) +{ + CK_OBJECT_HANDLE h; + CK_ATTRIBUTE attr = { CKA_ENCRYPT, &encrypt, sizeof(encrypt) }; + CK_RV rv; + + CHECK(find_label(session, "obj5", &h) == 1, "Key found"); + if (fail) + WP11_Test_StoreCloseFailAfter(0); + rv = funcList->C_SetAttributeValue(session, h, &attr, 1); + WP11_Test_StoreCloseFailAfter(-1); + CHECK_RV(rv, "Set CKA_ENCRYPT"); + return 0; +} + +/* Destroy an object with the store failing. The destroy is reported as + * failed and the object is gone from memory. */ +static int destroy_failing(CK_SESSION_HANDLE session, const char* label) +{ + CK_OBJECT_HANDLE h; + CK_RV rv; + + CHECK(find_label(session, label, &h) == 1, "Object found"); + WP11_Test_StoreCloseFailAfter(0); + rv = funcList->C_DestroyObject(session, h); + WP11_Test_StoreCloseFailAfter(-1); + CHECK(rv == CKR_FUNCTION_FAILED, "Destroy reports the failed store"); + CHECK(find_label(session, label, &h) == 0, "Object gone from memory"); + return 0; +} + +/* Rename an object with the store failing. The rename is live. */ +static int rename_failing(CK_SESSION_HANDLE session, const char* label, + const char* newLabel) +{ + CK_OBJECT_HANDLE h; + CK_ATTRIBUTE attr = { CKA_LABEL, (void*)newLabel, strlen(newLabel) }; + CK_RV rv; + + CHECK(find_label(session, label, &h) == 1, "Object found"); + WP11_Test_StoreCloseFailAfter(0); + rv = funcList->C_SetAttributeValue(session, h, &attr, 1); + WP11_Test_StoreCloseFailAfter(-1); + CHECK_RV(rv, "Rename object"); + CHECK(find_label(session, newLabel, &h) == 1, "Rename is live"); + return 0; +} + +/* A destroy or attribute change whose store fails leaves the records written + * before it loadable, and the next write that succeeds catches up. */ +static int store_fail_test(void) +{ + CK_SESSION_HANDLE session; + CK_OBJECT_HANDLE h; + static const char renamed[] = "obj1-renamed"; + int i; + + CHECK(clear_dir(STORE_DIR) == 0, "Clear store directory"); + CHECK_RV(pkcs11_init(), "Initialize"); + CHECK_RV(provision(), "Provision token"); + CHECK_RV(open_user_session(&session), "Open user session"); + /* Data objects 0 to 2, then the AES key (5), the newest. */ + for (i = 0; i <= 2; i++) + CHECK_RV(create_obj(session, i, &h), "Create object"); + CHECK_RV(create_obj(session, OBJ_AES, &h), "Create key"); + + /* The key would move into the place of object 0, but nothing is + * written, so the token loads as it was before the destroy. */ + CHECK(destroy_failing(session, "obj0") == 0, "Failed destroy"); + CHECK(reload_from_storage(session, 4) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(count_token_objects(session) == 4, "Four objects loaded"); + for (i = 0; i <= 2; i++) { + char label[16]; + + obj_label(i, label, sizeof(label)); + CHECK(check_data_obj(session, label, i) == 0, "Object loaded"); + } + CHECK(check_key(session, CK_TRUE) == 0, "Key loaded"); + + /* Again, then a create catches up with the destroy. */ + CHECK(destroy_failing(session, "obj0") == 0, "Failed destroy"); + CHECK_RV(create_obj(session, 6, &h), "Create object"); + CHECK(reload_from_storage(session, 4) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(count_token_objects(session) == 4, "Four objects loaded"); + CHECK(find_label(session, "obj0", &h) == 0, "Destroyed object gone"); + CHECK(check_data_obj(session, "obj1", 1) == 0, "Object loaded"); + CHECK(check_data_obj(session, "obj2", 2) == 0, "Object loaded"); + CHECK(check_data_obj(session, "obj6", 6) == 0, "Object loaded"); + CHECK(check_key(session, CK_TRUE) == 0, "Moved key loaded"); + + /* A rename whose store fails is not stored. */ + CHECK(rename_failing(session, "obj1", renamed) == 0, "Failed rename"); + CHECK(reload_from_storage(session, 4) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(check_data_obj(session, "obj1", 1) == 0, "Old label stored"); + CHECK(find_label(session, renamed, &h) == 0, "New label not stored"); + + /* Again, then a create catches up with the rename. */ + CHECK(rename_failing(session, "obj1", renamed) == 0, "Failed rename"); + CHECK_RV(create_obj(session, 7, &h), "Create object"); + CHECK(reload_from_storage(session, 5) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(check_data_obj(session, renamed, 1) == 0, "Rename stored"); + CHECK(find_label(session, "obj1", &h) == 0, "Old label gone"); + + /* A policy change on the key is stored and its value still loads. */ + CHECK(set_key_encrypt(session, CK_FALSE, 0) == 0, "Clear CKA_ENCRYPT"); + CHECK(reload_from_storage(session, 5) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(check_key(session, CK_FALSE) == 0, "Changed key loaded"); + + /* One whose store fails leaves the stored key as it was. */ + CHECK(set_key_encrypt(session, CK_TRUE, 1) == 0, "Set CKA_ENCRYPT"); + CHECK(reload_from_storage(session, 5) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(check_key(session, CK_FALSE) == 0, "Unchanged key loaded"); + + /* Again, then a create catches up with the policy change. */ + CHECK(set_key_encrypt(session, CK_TRUE, 1) == 0, "Set CKA_ENCRYPT"); + CHECK_RV(create_obj(session, 3, &h), "Create object"); + CHECK(reload_from_storage(session, 6) == 0, "Reload"); + CHECK_RV(open_user_session(&session), "Open user session"); + CHECK(check_key(session, CK_TRUE) == 0, "Changed key loaded"); + CHECK(check_data_obj(session, renamed, 1) == 0, "Renamed object"); + CHECK(check_data_obj(session, "obj3", 3) == 0, "Object loaded"); + + (void)funcList->C_Logout(session); + (void)funcList->C_CloseSession(session); + pkcs11_final(); + return 0; +} +#endif /* DEBUG_WOLFPKCS11 */ + #endif /* RUN_TEST */ int main(int argc, char* argv[]) @@ -604,6 +778,12 @@ int main(int argc, char* argv[]) printf("Token pause and resume test\n"); ret = pause_resume_test(); } +#ifdef DEBUG_WOLFPKCS11 + if (ret == 0) { + printf("Token store failure test\n"); + ret = store_fail_test(); + } +#endif if (ret == 0) printf("All tests passed!\n"); return (ret == 0) ? 0 : 1;