LMS/XMSS: don't skip the software reload when a read callback is set - #11059
Merged
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11059
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Contributor
Author
|
Jenkins retest this please |
Member
|
Jenkins retest this please "PRB-fips-ready-config'" |
Member
|
Jenkins retest this please |
dgarske
requested changes
Aug 5, 2026
wc_LmsKey_Reload and wc_XmssKey_Reload return success without doing any work whenever key->devId != INVALID_DEVID, on the assumption that a device-bound key has its private state inside that device. That assumption does not hold for a caller that sets devId only to route primitives to a hardware accelerator while keeping the key state in its own storage. A wolfHSM server is exactly that caller. It configures a server-wide devId so AES, ECC and RSA reach the platform accelerator, then for LMS/XMSS it installs read/write callbacks and calls Reload to rebuild the expanded private key before signing. With a hardware devId configured, Reload returned 0 immediately and the key was left unusable for the first sign that follows. The failure needs no invalid input, only a server built with an accelerator. For LMS that first sign is a crash: key->priv_data stays NULL, wc_hss_sign finds priv.inited clear and calls wc_hss_init_auth_path, which derives its first read from a NULL priv pointer. On a target without a mapping at low addresses that is a bus fault. For XMSS the skipped reload never allocates key->sk, leaving both the pointer NULL and sk_len 0, so the outcome depends on the caller's read callback: one that honours the length it is given returns nothing and the sign fails with IO_FAILED_E, while one that writes a fixed-size record faults on the NULL destination. Key generation was unaffected and hid the problem: wc_LmsKey_MakeKey already treats devId as "offer the operation to the callback, fall back to software on CRYPTOCB_UNAVAILABLE", so it populates the key correctly when the accelerator declines. Reload had no equivalent fall-through, so the same key and the same devId were interpreted two different ways by the same API. Qualify the short-circuit with key->read_private_key == NULL. A caller that has installed a read callback is asking for the software reload to fetch the state through it, whereas a genuinely device-backed key installs no such callback. Key generation keeps offering the operation to the crypto callback, so a port with real stateful-hash-signature hardware is not prevented from using it. The reference POSIX wolfHSM server runs with INVALID_DEVID, which is why this was not caught by existing tests. test_wc_LmsKey_reload_devid and test_wc_XmssKey_reload_devid cover both arms of the new condition: a key whose read callback is set must come back from Reload with its private key expanded (priv_data for LMS, sk for XMSS) and able to sign, while a key on the same devId with no read callback must still short-circuit. Both tests register a crypto callback that declines every operation with CRYPTOCB_UNAVAILABLE, which is the accelerator this fix is about. Asserting on the expanded key means the old behaviour fails the assertion rather than the NULL dereference it leads to. The rest of the LMS and XMSS suite is unaffected, as every other key there uses INVALID_DEVID. The XMSS test needs the H10 SHA-256 parameter set, which is only in the algorithm table when both the hash and the height are compiled in, so it carries a guard for that. The crypto callback's own guard is the exact union of the two test guards, or a build with only one of the two algorithms would emit it with no caller and fail -Werror. Both the Reload implementation comments and the published Doxygen now state that the read callback, not the devId, decides whether the software reload runs.
Frauschi
force-pushed
the
sfhb_reload_fix
branch
from
August 5, 2026 20:23
99d4830 to
7a3d424
Compare
dgarske
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
wc_LmsKey_Reloadandwc_XmssKey_Reloadreturn success without doing any work wheneverkey->devId != INVALID_DEVID, assuming a device-bound key keeps its private state inside the device. That does not hold for a caller that sets a devId only to route primitives to a hardware accelerator while keeping the key state in its own storage - a wolfHSM server configures a server-wide devId for AES/ECC/RSA, then installs LMS/XMSS read/write callbacks and callsReloadbefore signing.With such a devId configured,
Reloadreturned 0 immediately and left the key unusable for the first sign that follows:priv_datastays NULL,wc_hss_signfindspriv.initedclear and callswc_hss_init_auth_path, which derives its first read from a NULLprivpointer - a bus fault on a target with nothing mapped at low addresses.key->skis never allocated (sk_lenstays 0), so the sign fails withIO_FAILED_Efor a read callback that honours the length it is given, and faults for one that writes a fixed-size record.Key generation hid the problem:
wc_LmsKey_MakeKeyalready treats devId as "offer the operation to the callback, fall back to software onCRYPTOCB_UNAVAILABLE".Reloadhad no equivalent fall-through - and no cryptocb reload op exists to offer it to - so the same key and the same devId were interpreted two different ways by the same API.The fix qualifies the short-circuit with
key->read_private_key == NULL: a caller that installed a read callback is asking for the software reload to fetch state through it, while a genuinely device-backed key installs no such callback. Key generation still offers the operation to the crypto callback, so a port with real stateful-hash-signature hardware is unaffected.Testing
test_wc_LmsKey_reload_devidandtest_wc_XmssKey_reload_devidcover both arms of the new condition, driven by a crypto callback that declines every operation withCRYPTOCB_UNAVAILABLE(the accelerator this fix is about):Reloadmust come back with the private key expanded (priv_data/sk), and sign/verify must succeed;Reloadmust still short-circuit and leave the key state in the device.Asserting on the expanded key means the old behaviour fails the assertion rather than the NULL dereference it leads to. Verified against
./configure --enable-lms --enable-xmss --enable-cryptocb --enable-debug: both tests fail before the fix and pass after it, with the rest of thelmsandxmssgroups unaffected (every other key there usesINVALID_DEVID). Also builds clean in an--enable-alltree, where the whole group compiles out.The public Doxygen for both Reload functions now documents that the read callback, not the devId, decides whether the software reload runs, and that a
0return can mean nothing was loaded.