Add operation timeout - step 1 #220

miyazakh · 2025-10-18T01:36:43Z

Add crypto timeout to RNG and AES

The changes add timeout control logic to cryptographic and other operations under the wolfHSM framework. The modifications allow the client to enforce a maximum allowed time for operations.

Added new callback function pointers to the relevant configuration/context structures to support timeout handling. These callbacks allow the application to provide custom time-related functions, such as:

Getting the current time (GetCurrentTime)
Checking whether the timeout has expired (CheckTimeout)

When the crypt-timeout feature is enabled, the GetCurrentTime callback must be provided as a user-defined function. If the CheckTimeout callback is not defined, internal default implementation is used.

Added a new build-time configuration macro:

WOLFHSM_CFG_ENABLE_CLIENT_TIMEOUT

This macro enables the client-side timeout feature. When enabled, the wolfHSM client checks for timeout conditions during operations.

The feature has been added to RNG and AES. It will be extended to the remaining cryptographic algorithms once this PR is approved.

For testing, make CRYPTIMEOUT=1 enables the items in tests/ folder.

douzzer · 2025-10-31T05:01:23Z

@miyazakh conflicts to resolve

miyazakh · 2025-11-02T00:07:48Z

resolve conflicts and issues after rebase. Updated based on Copilot’s suggestions

miyazakh · 2025-11-05T00:10:23Z

Hi @billphipps and @bigbrett
Would a generic timeout framework be acceptable to you? If so, I will proceed with adding the feature to the remaining cryptographic algorithms.

bigbrett

Hi Hide,

Thanks for this, and great work! Overall I think the architecture matches my expectations for the most part, however I have proposed some changes to hopefully allow this feature to scale beyond just crypto. I think we should get this all correct and working as desired before moving on to other algorithms.

Also, please remove all braceless if statements from this PR.

bigbrett · 2025-11-05T14:43:12Z

wolfhsm/wh_comm.h

+     */
+    uint64_t crypt_start_time;
+    /* cached conversion of crypt_timeout */
+    uint64_t crypt_timeout_ms;


do we want to hardcode the units into this? What if I want us level timeout?

Refactor to remove the "_ms" suffix from variable names. The time unit depends on the return value of GetTimeout().

bigbrett · 2025-11-05T14:45:05Z

wolfhsm/wh_comm.h

    whCommSetConnectedCb connect_cb;
    uint8_t client_id;
+#if defined(WOLFHSM_CFG_ENABLE_CLIENT_CRYPTIMEOUT)
+    uint8_t crypt_timeout_enabled;


why is the enable flag in the config struct? IMO we should be able to turn on/off dynamically. We should have an API function in this file to set the enable flag so that it can be turned on/off on a per-operation basis.

Add wh_Client_timeoutEnable. user can enable / disable time out feature through the API.

bigbrett · 2025-11-05T14:47:12Z

wolfhsm/wh_comm.h

+     * callback when the operation started.
+     * The actual unit depends on the GetCurrentTime() implementation.
+     */
+    uint64_t crypt_start_time;


Could we have all timeout state stored in its own structure?

something like whClientTimeOutContext

Add whClientTimeOutContext definition in wh_client.h

bigbrett · 2025-11-05T14:48:07Z

wolfhsm/wh_comm.h

+     */
+    int (*CheckTimeout)(uint64_t* start_time, uint64_t timeout_ms);
+    uint8_t  WH_PAD[8];
+} whCryptoClientTimeOutCb;


I think we want this feature to be generic and eventually scale beyond crypto so lets name it accordingly. We might want NVM or keystore operations to timeout, for exampe. No reason to box this into crypto from the start.

Remove crypt and be generic.

bigbrett · 2025-11-05T14:49:08Z

wolfhsm/wh_comm.h

+#ifdef WOLFSSL_TIMEVAL
+    typedef WOLFSSL_TIMEVAL WOLFHSM_TIMEVAL;
+#else
+    typedef struct {
+        uint64_t tv_sec;  /* Seconds. */
+        uint64_t tv_usec; /* Microseconds. */
+    } WOLFHSM_TIMEVAL;
+#endif


I don't like the all-caps naming nor the dependency on the wolfSSL implementation of the structure. I think it is fine to just define our own whTimeVal structure, as we don't want it to hinge on the particular configuration of wolfSSL

Renamed WOLFHSM_TIMEVALP to wh_timeval

bigbrett · 2025-11-05T15:04:04Z

test/config/wolfhsm_cfg.h

+/* Enable client crypto timeout feature for testing */
+#if defined(WOLFHSM_CFG_ENABLE_CLIENT_CRYPTIMEOUT) && \
+    defined(WOLFHSM_CFG_TEST_POSIX)
+#define WOLFHSM_CFG_CLIENT_CRYPTIMEOUT_SEC (2)


2 seconds? seems long for a default value on a POSIX system

Changed to WOLFHSM_CFG_CLIENT_TIMEOUT_USEC 500000 /* 500 ms */

bigbrett · 2025-11-05T15:13:11Z

test/wh_test_wolfcrypt_test.c

+        .transport_context = (void*)tmcc,
+        .transport_config  = (void*)tmcf,
+#if defined(WOLFHSM_CFG_TEST_CLIENT_CRYPTIMEOUT)
+        .crypt_timeout_cb = (void*)tc_timeoutcb,


As mentioned in another comment, we want all timeout state condensed to one structure in the client comm context. Same with in the config struct. That way if the config struct has a NULL element for the timeout substruct, those fields won't be populated in the context. The context struct should be allowed to be NULL (optionally set) and no operation should happen in this case (but it should be safe to leave uninitialized). All accesses should be gated on NULL checks.

That way, in a scenario like the wolfCrypt test runner here, where we don't care about using timeouts, we can just leave this as NULL In the config.

bigbrett · 2025-11-05T15:30:35Z

test/wh_test_crypto.c

+
+    WH_TEST_RETURN_ON_FAIL(wh_Client_Init(client, config));
+    client->comm->client_id = ALT_CLIENT_ID_2;
+    ret                     = wh_Client_CommInit(client, NULL, NULL);


Why are we changing the client ID here after initialization? I'm confused as to what you are trying to accomplish.

Remove whTest_CryptoClientConfig_Timeout() and integrate tests into the existing crypt test

bigbrett · 2025-11-05T15:31:53Z

test/wh_test_crypto.c

+
+    if (ret == 0) {
+        /* expect to have TIMEOUT */
+        ret = whTest_CryptoRng(client, WH_DEV_ID, rng);


Ideally we could do things like set the timeout for the operations done as part of the RNG tests, then clear it after. Theoretically we could test different timeouts for different algo tests.

bigbrett · 2025-11-05T15:33:48Z

test/wh_test_crypto.c

I think the serverDelay logic here is confusing and hard to follow. Originally we used the serverDelay flag to block the server until released by the client (for CMAC cancellation tests). I think this is a good time to refactor this functionality to support both use cases in a cleaner manner. Instead of setting delay values from the client side, could we do all this by just keeping it as a binary pause/resume that the client thread can signal to the server? AFAICT the client doesn't really care about the "time" we delay the server for, but more that we "pause it" while we do something. In this case we would just "pause the server", send the request, hit our timeout and verify it works as expected, then can "resume" the server after. Or did you have a different intent?

Agree with you. Refactored while(serverDelay) to use pthread_cond_wait if POSIX.

bigbrett

Overall I'm still not happy with the test integration. It adds more boilerplate to the crypto tests than I am comfortable with and makes them difficult to read. I'd prefer timeout functionality to be tested in its own standalone test if this is the case.

bigbrett · 2025-11-10T21:36:33Z

wolfhsm/wh_client.h

+     */
+    int (*CheckTimeout)(uint64_t* start_time, uint64_t timeout_val);
+} whClientTimeOutCb;
+typedef struct {


add newline between definitions

bigbrett · 2025-11-10T21:36:38Z

wolfhsm/wh_client.h

+    uint64_t tv_sec;  /* Seconds. */
+    uint64_t tv_usec; /* Microseconds. */
+} wh_timeval;
+typedef struct {


add newline between definitions

Added newline

bigbrett · 2025-11-11T21:49:08Z

wolfhsm/wh_comm.h

    int (*Cleanup)(void* context);
 } whTransportClientCb;

+


extra whitespace

bigbrett · 2025-11-11T21:50:07Z

wolfhsm/wh_client.h

+int wh_Client_CheckTimeout(whClientContext* context);
+/* Register Client Timeout Callback */
+int wh_Client_timeoutRegisterCb(whClientContext* client,
+                                            whClientTimeOutCb* cb);


bigbrett · 2025-11-11T21:50:24Z

wolfhsm/wh_client.h

+                                            whClientTimeOutCb* cb);
+/* Enable Client Timeout */
+int wh_Client_timeoutEnable(whClientContext* client,
+                                            wh_timeval* timeout_val);


bigbrett · 2025-11-11T22:35:20Z

src/wh_client.c

+                + (uint64_t)((tv->tv_usec) / WH_BASE_TIMEOUT_UNIT);
+}
+/* Set Time Out if needed */
+int wh_Client_InitCryptTimeout(whClientContext* c)


This is a confusing name - I would rename to wh_Client_TimeoutStart()

bigbrett · 2025-11-11T22:35:35Z

src/wh_client.c

+}
+
+/* Check Crypto Timeout */
+int wh_Client_CheckTimeout(whClientContext* c)


Rename to wh_Client_TimeoutCheck

bigbrett · 2025-11-11T22:35:53Z

src/wh_client.c

+    return WH_ERROR_OK;
+}
+
+int wh_Client_timeoutRegisterCb(whClientContext* client,


missing capital T for consistency

Capitalized

bigbrett · 2025-11-11T22:37:03Z

src/wh_client.c

+    return WH_ERROR_OK;
+}
+
+int wh_Client_timeoutEnable(whClientContext* client,


I would rename to wh_Client_TimeoutSet()

bigbrett · 2025-11-11T22:48:09Z

src/wh_client_crypto.c


    return header->rc;
 }
+static int _wait_response_with_crypttimeout(whClientContext *ctx,


We should be able to refactor this to include the send too, right? That way we have one function that sets up the timeout and sends a request and blocks on the response?

I'd name this new function _SendRecieveWithTimeout()

Renamed and included sends and Start

bigbrett · 2025-11-12T14:10:55Z

src/wh_client_crypto.c

    wh_Utils_Hexdump("[client] req packet: \n", (uint8_t*)req, req_len);
 #endif
    ret = wh_Client_SendRequest(ctx, group, action, req_len, dataPtr);
+#if defined(WOLFHSM_CFG_ENABLE_CWOLFHSM_CFG_CLIENT_TIMEOUTLIENT_TIMEOUT)


broken macro

bigbrett · 2025-11-12T14:14:10Z

src/wh_client.c

+static uint64_t wh_timeval_to_64(const wh_timeval* tv)
+{
+    if (tv == NULL) return 0;
+    return (uint64_t)tv->tv_sec * WH_BASE_TIMEOUT_UNIT
+                + (uint64_t)((tv->tv_usec) / WH_BASE_TIMEOUT_UNIT);
+}


I believe this will result in a loss of precision. That only works for the default millisecond unit (1000). If a platform overrides WH_BASE_TIMEOUT_UNIT (e.g., 1 for seconds or 1'000'000 for microseconds) the microsecond field is either treated as seconds (tv_usec / 1) or dropped to zero (tv_usec / 1'000'000), so sub‑second timeouts are wildly wrong or become 0 (no timeout). I think the conversion should scale by the ratio of microseconds per base unit (e.g., tv_usec * WH_BASE_TIMEOUT_UNIT / 1000000) to keep behavior consistent when the base unit is customized.

All that said, I'm fine with the timeout being in us to simplify all of this.

tv_usec * WH_BASE_TIMEOUT_UNIT / 1000000 is true. Fixed it.

I'm not sure we need microsecond accuracy here, so I'll leave it as milliseconds for now.

Updated based on Copilot's suggestions

bigbrett · 2025-11-19T21:21:35Z

@miyazakh this is mostly good, however I am also working on a feature that needs a system time abstraction. Rather than each feature that needs it using callbacks, I think we need to generalize wolfHSM to have a built-in "get time" functionality. That way this can be used directly by benchmarks, timeouts, and other features like logging. I'm going to take a stab at this in my logging PR that I'm about to open then will ping you when it is time to integrate into the timeout feature. It should hopefully simplify the code you need to write. So putting this on hold for now.

miyazakh self-assigned this Oct 18, 2025

miyazakh force-pushed the timeout branch from 4b1af43 to fe1a54f Compare October 18, 2025 01:58

miyazakh assigned wolfSSL-Bot and unassigned miyazakh Oct 18, 2025

dgarske requested review from bigbrett and billphipps October 20, 2025 16:16

dgarske assigned bigbrett Oct 21, 2025

douzzer assigned miyazakh and unassigned bigbrett and wolfSSL-Bot Oct 31, 2025

miyazakh force-pushed the timeout branch from fe1a54f to ee7e9ba Compare November 2, 2025 00:07

miyazakh force-pushed the timeout branch from ee7e9ba to 7cae6bc Compare November 2, 2025 00:10

miyazakh assigned wolfSSL-Bot and unassigned miyazakh Nov 2, 2025

bigbrett assigned bigbrett and billphipps and unassigned wolfSSL-Bot Nov 3, 2025

miyazakh force-pushed the timeout branch from 7cae6bc to bedda8a Compare November 4, 2025 22:37

bigbrett requested changes Nov 5, 2025

View reviewed changes

bigbrett removed their assignment Nov 5, 2025

miyazakh self-assigned this Nov 5, 2025

miyazakh force-pushed the timeout branch from 6469c66 to 02aa2c4 Compare November 8, 2025 03:27

miyazakh changed the title ~~Add crypto timeout - step 1~~ Add operation timeout - step 1 Nov 8, 2025

miyazakh removed their assignment Nov 8, 2025

bigbrett requested changes Nov 11, 2025

View reviewed changes

bigbrett requested changes Nov 12, 2025

View reviewed changes

bigbrett assigned miyazakh Nov 12, 2025

miyazakh force-pushed the timeout branch from 02aa2c4 to 43e8c75 Compare November 14, 2025 10:40

miyazakh added 5 commits November 15, 2025 06:52

Add crypto timeout to RNG and AES

c96af55

Updated based on Copilot's suggestions

addressed code review

34db12a

Addressed code review

39539ff

apply clang-format

ccd25e0

fix scan build failure

5935652

miyazakh force-pushed the timeout branch from d1c29eb to 5935652 Compare November 14, 2025 21:52

miyazakh removed their assignment Nov 14, 2025

miyazakh marked this pull request as draft November 19, 2025 22:59

Add operation timeout - step 1 #220

Are you sure you want to change the base?

Add operation timeout - step 1 #220

Uh oh!

Conversation

miyazakh commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douzzer commented Oct 31, 2025

Uh oh!

miyazakh commented Nov 2, 2025

Uh oh!

miyazakh commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bigbrett left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miyazakh Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigbrett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miyazakh commented Oct 18, 2025 •

edited

Loading

miyazakh commented Nov 5, 2025 •

edited

Loading

bigbrett left a comment •

edited

Loading

miyazakh Nov 8, 2025 •

edited

Loading