The store commits sectors with hal_flash_erase()/hal_flash_write() and
then reads them back through the memory map - sector_ptr(),
cache_get_sector()'s refill, and the raw magic reads in check_vault().
On a part that caches flash reads (STM32 ICACHE) those reads can return
pre-erase bytes. check_vault() is the worst case: a stale magic there
does not merely read wrong, it triggers restore_backup() or a full vault
re-initialisation, losing the token.
Adds power-fail injection to the flash mock and a test that cuts power at
every flash operation of a rewrite window, asserting the object always reads
back as the whole old payload, the whole new payload, or empty. Fails at
op 3 without this fix, passes with it.
unit-pkcs11_store 10/10; full unit-tests suite green.
test_concurrent_reader_sees_pending_writes only ever read from flash:
the reader's Store_Open calls check_vault(), which flushes the sector
cache, so the sector_ptr() read path in Store_Read was never exercised
and the test passed identically against the pre-PR memcpy.
Write more on the still-open writer after the reader is open. That
batch lands only in the sector cache, so the reader can only see it
through the cached read path and the live header size; a flash-only or
snapshot-size read returns EOF here. Verified: the new assertion fails
against the pre-fix store (ret == 0) and passes with the live-size fix.
Addresses PR #873 review comments (wolfSSL-Fenrir-bot,
tools/unit-tests/unit-pkcs11_store.c:587, both near-duplicate findings).
Store_Read and Store_Write used handle->size, a snapshot taken at
Store_Open. The payload path reads through the sector cache, so once
another window's batch (e.g. a write-open truncation) sat pending in
the cache, the window saw live erased data under a stale size and
returned 0xFF bytes past the true end instead of EOF. Pre-PR the size
was read live from the flash header on every call, so the PR regressed
that case.
Read the size from the same (possibly cached) header sector the payload
comes from, via store_live_size(), so size and data share one source of
truth. Drop the now-dead handle->size snapshot; update_store_size()
only writes the cached header node.
Addresses PR #873 review comment (wolfSSL-Fenrir-bot,
src/pkcs11_store.c:711).
check_vault() dropped the shared sector cache on every vault
validation, silently losing the pending writes of any still-open
window when another handle was opened or an object removed
(MAX_OPEN_STORES allows 16). Flush instead - the atomic header-last
commit - so an in-flight batch only gets an earlier commit point;
its data is never discarded.
wolfPKCS11_Store_Read() now reads through sector_ptr() like every
other read in the file, so a sector still in the cache can never be
read stale against a live size.
Add unit tests covering the interleaved-window data loss and a
concurrent reader observing a pending write; both fail without the
check_vault fix.
Every wolfPKCS11 field write flushed the payload sector and the header
sector to flash (2 erases + 2 programs of a full sector each), and the
token store re-serializes all objects per C_CreateObject/C_DestroyObject,
so those calls cost hundreds of sector erases and tens of seconds on
flash with slow erase times.
Cache modified sectors in RAM and commit them together when the store
window closes:
- sector cache sized to the worst-case span of one object plus the
header sector (WOLFBOOT_PKCS11_STORE_CACHE_SECTORS), LRU eviction
when exceeded
- header sector commits last, so a committed header is the atomic
commit point of the batch: power failure during a flush leaves the
flash in either the pre-batch or the post-batch state
- per-commit backup sector write preserved, keeping recovery of the
sector in flight at failure time
- delete_object commits on return (durability contract, unit-tested)
- nodes table, bitmap, payload ids and the live object size
(handle->size) are read from the cache when the sector is dirty
Measured on an STM32H5 with 8KB sectors, wolfPKCS11 in the secure
world: C_CreateObject 1.5s -> 0.15s, C_DestroyObject 1.3s -> 0.12s,
456 -> 40 sector erases per create, and the count no longer scales
with the number of objects in the token.
PKCS11_STORE_STATS (off by default) adds flash-activity counters and a
test-app bench to quantify store traffic: make PKCS11_STORE_STATS=1.
The PKCS#11 and PSA store Remove paths invalidated the metadata and
freed the bitmap slot but left the payload in flash, so removed keys
stayed recoverable by a physical reader. Both Remove paths now call
erase_object_payload() before invalidating the metadata; the existing
sector read-modify-write preserves neighboring slots. Raw-flash
deletion tests added to both unit suites.
wolfPKCS11/PSA_Store_Read/Write added int len to unsigned
in_buffer_offset before validation: a sufficiently negative len
wrapped, hit the truncation branch, and was replaced by the remaining
object or capacity bytes, bypassing Write's later len < 0 guard.
Reject len < 0 first in all four functions (post-clamp guard kept).
When an existing object was reopened in write mode, only hdr->size was
reset; the data sectors were never erased. A subsequent shorter write
left the prior key bytes in flash beyond hdr->size (physically readable
via JTAG/SWD). Mirror the fix applied to psa_store in F-4784: track
is_new, and when opening a pre-existing object in write mode erase every
sector of the object region via cache_commit (0xFF fill, tok/obj id
re-written in sector 0) before the size record is truncated.
A power fault during cache_commit(0) can leave a node header in the
keyvault with valid magic/tok/obj/type but pos left as erased flash
(0xFFFFFFFF). On the next boot find_object_buffer() detects the
data-sector mismatch and calls delete_object(), which reaches
bitmap_put(0xFFFFFFFF, 0). bitmap_put computed octet = pos/8 and indexed
cached_sector[4 + octet] with no bounds check, writing ~512 MB past the
static sector buffer. Reject pos >= KEYVAULT_MAX_ITEMS so a corrupted
header can no longer turn into an out-of-bounds write.
Adds a unit test that seeds a node with pos=PKCS11_INVALID_ID and
confirms delete_object() no longer faults.
Daniele Lacamera
gojimmypi
Andrew Hutchings
Daniele Lacamera