-
Notifications
You must be signed in to change notification settings - Fork 22
Add operation timeout - step 1 #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,7 +80,13 @@ int wh_Client_Init(whClientContext* c, const whClientConfig* config) | |
| /* register the cancel callback */ | ||
| c->cancelCb = config->cancelCb; | ||
| #endif | ||
|
|
||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| if (NULL != config->timeoutConfig) { | ||
| c->timeout.timeout_val = config->timeoutConfig->timeout_val; | ||
| c->timeout.timeout_enabled = config->timeoutConfig->timeout_enabled; | ||
| c->timeout.start_time = 0; | ||
| } | ||
| #endif | ||
| rc = wh_CommClient_Init(c->comm, config->comm); | ||
|
|
||
| #ifndef WOLFHSM_CFG_NO_CRYPTO | ||
|
|
@@ -1523,4 +1529,104 @@ int wh_Client_KeyExportDma(whClientContext* c, uint16_t keyId, | |
| } | ||
| #endif /* WOLFHSM_CFG_DMA */ | ||
|
|
||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| 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); | ||
| } | ||
| /* Set Time Out if needed */ | ||
| int wh_Client_InitCryptTimeout(whClientContext* c) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a confusing name - I would rename to |
||
| { | ||
| if (c == NULL) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| /* if feature not enabled, nothing to do */ | ||
| if (c->timeout.timeout_enabled != 1) { | ||
| return WH_ERROR_OK; | ||
| } | ||
| if (c->timeout.cb.GetCurrentTime == NULL) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
| /* initialize start time */ | ||
| c->timeout.start_time = c->timeout.cb.GetCurrentTime(1); | ||
|
|
||
| return WH_ERROR_OK; | ||
| } | ||
|
|
||
| /* Check Crypto Timeout */ | ||
| int wh_Client_CheckTimeout(whClientContext* c) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to |
||
| { | ||
| uint64_t current_ = 0; | ||
| uint64_t elapsed_ = 0; | ||
| uint64_t timeout_ = 0; | ||
|
|
||
| if (c == NULL) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| if (c->timeout.timeout_enabled != 1) { | ||
| return WH_ERROR_OK; | ||
| } | ||
|
|
||
| if (c->timeout.cb.GetCurrentTime == NULL) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| timeout_ = wh_timeval_to_64(&c->timeout.timeout_val); | ||
| if (timeout_ == 0) { | ||
| return WH_ERROR_OK; | ||
| } | ||
|
|
||
| /* check timeout by user cb if defined */ | ||
| if (c->timeout.cb.CheckTimeout != NULL) { | ||
| return c->timeout.cb.CheckTimeout( | ||
| &c->timeout.start_time, timeout_); | ||
| } | ||
|
|
||
| /* Otherwise compute elapsed using user-provided GetCurrentTime */ | ||
| current_ = c->timeout.cb.GetCurrentTime(0); | ||
| elapsed_ = current_ - c->timeout.start_time; | ||
| if (elapsed_ > timeout_) { | ||
| return WH_ERROR_TIMEOUT; | ||
| } | ||
|
|
||
| return WH_ERROR_OK; | ||
| } | ||
|
|
||
| int wh_Client_timeoutRegisterCb(whClientContext* client, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing capital T for consistency |
||
| whClientTimeOutCb* cb) | ||
| { | ||
| /* No NULL check for cb, since it is optional and always NULL checked before | ||
| * it is called */ | ||
| if (NULL == client) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| client->timeout.cb.GetCurrentTime = cb->GetCurrentTime; | ||
| client->timeout.cb.CheckTimeout = cb->CheckTimeout; | ||
|
|
||
| return WH_ERROR_OK; | ||
| } | ||
|
|
||
| int wh_Client_timeoutEnable(whClientContext* client, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rename to |
||
| wh_timeval* timeout_val) | ||
| { | ||
| if (NULL == client) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| if (timeout_val != NULL) { | ||
| client->timeout.timeout_enabled = 1; | ||
| memcpy(&client->timeout.timeout_val, timeout_val, | ||
| sizeof(wh_timeval)); | ||
| } else { | ||
| client->timeout.timeout_enabled = 0; | ||
| memset(&client->timeout.timeout_val, 0, sizeof(wh_timeval)); | ||
| } | ||
| return WH_ERROR_OK; | ||
| } | ||
| #endif /* WOLFHSM_CFG_CLIENT_TIMEOUT */ | ||
| #endif /* WOLFHSM_CFG_ENABLE_CLIENT */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,6 +180,27 @@ static int _getCryptoResponse(uint8_t* respBuf, uint16_t type, | |
|
|
||
| return header->rc; | ||
| } | ||
| static int _wait_response_with_crypttimeout(whClientContext *ctx, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| uint16_t *out_group, uint16_t *out_action, | ||
| uint16_t *out_size, void* data) | ||
| { | ||
| int ret = WH_ERROR_OK; | ||
| do { | ||
| ret = wh_Client_RecvResponse(ctx, out_group, out_action, out_size, data); | ||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_NOTREADY) { | ||
| /* Check for crypto timeout */ | ||
| int chk = wh_Client_CheckTimeout(ctx); | ||
| if (chk == WH_ERROR_TIMEOUT) { | ||
| return WH_ERROR_TIMEOUT; | ||
| } else if (chk < 0 && chk != WH_ERROR_OK) { | ||
| return chk; | ||
| } | ||
| } | ||
| #endif | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
| return ret; | ||
| } | ||
|
|
||
| /** Implementations */ | ||
| int wh_Client_RngGenerate(whClientContext* ctx, uint8_t* out, uint32_t size) | ||
|
|
@@ -233,9 +254,14 @@ int wh_Client_RngGenerate(whClientContext* ctx, uint8_t* out, uint32_t size) | |
|
|
||
| /* Send request and get response */ | ||
| ret = wh_Client_SendRequest(ctx, group, action, req_len, dataPtr); | ||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| if (ret == 0) { | ||
| do { | ||
| ret = wh_Client_RecvResponse(ctx, &group, &action, &res_len, | ||
| ret = _wait_response_with_crypttimeout(ctx, &group, &action, &res_len, | ||
| dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
| } | ||
|
|
@@ -420,14 +446,21 @@ int wh_Client_AesCtr(whClientContext* ctx, Aes* aes, int enc, const uint8_t* in, | |
| 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_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| /* read response */ | ||
| if (ret == WH_ERROR_OK) { | ||
| /* Response packet */ | ||
| uint16_t res_len = 0; | ||
| do { | ||
| ret = | ||
| wh_Client_RecvResponse(ctx, &group, &action, &res_len, dataPtr); | ||
| _wait_response_with_crypttimeout(ctx, &group, &action, | ||
| &res_len, dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
|
|
||
| if (ret == WH_ERROR_OK) { | ||
| ret = _getCryptoResponse(dataPtr, type, (uint8_t**)&res); | ||
| if (ret == WH_ERROR_OK) { | ||
|
|
@@ -542,14 +575,21 @@ int wh_Client_AesEcb(whClientContext* ctx, Aes* aes, int enc, const uint8_t* in, | |
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. broken macro |
||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| /* read response */ | ||
| if (ret == WH_ERROR_OK) { | ||
| /* Response packet */ | ||
| uint16_t res_len = 0; | ||
| do { | ||
| ret = | ||
| wh_Client_RecvResponse(ctx, &group, &action, &res_len, dataPtr); | ||
| _wait_response_with_crypttimeout(ctx, &group, &action, | ||
| &res_len, dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
|
|
||
| if (ret == WH_ERROR_OK) { | ||
| ret = _getCryptoResponse(dataPtr, type, (uint8_t**)&res); | ||
| if (ret == WH_ERROR_OK) { | ||
|
|
@@ -661,14 +701,21 @@ int wh_Client_AesCbc(whClientContext* ctx, Aes* aes, int enc, const uint8_t* in, | |
| 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_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| /* read response */ | ||
| if (ret == WH_ERROR_OK) { | ||
| /* Response packet */ | ||
| uint16_t res_len = 0; | ||
| do { | ||
| ret = | ||
| wh_Client_RecvResponse(ctx, &group, &action, &res_len, dataPtr); | ||
| _wait_response_with_crypttimeout(ctx, &group, &action, | ||
| &res_len, dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
|
|
||
| if (ret == WH_ERROR_OK) { | ||
| ret = _getCryptoResponse(dataPtr, type, (uint8_t**)&res); | ||
| if (ret == WH_ERROR_OK) { | ||
|
|
@@ -795,11 +842,17 @@ int wh_Client_AesGcm(whClientContext* ctx, Aes* aes, int enc, const uint8_t* in, | |
|
|
||
| /* Send request and receive response */ | ||
| ret = wh_Client_SendRequest(ctx, group, action, req_len, dataPtr); | ||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| if (ret == 0) { | ||
| uint16_t res_len = 0; | ||
| do { | ||
| ret = | ||
| wh_Client_RecvResponse(ctx, &group, &action, &res_len, dataPtr); | ||
| _wait_response_with_crypttimeout(ctx, &group, &action, | ||
| &res_len, dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
|
|
||
| if (ret == WH_ERROR_OK) { | ||
|
|
@@ -1005,11 +1058,17 @@ int wh_Client_AesGcmDma(whClientContext* ctx, Aes* aes, int enc, | |
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_SendRequest(ctx, group, action, reqLen, dataPtr); | ||
| } | ||
| #if defined(WOLFHSM_CFG_CLIENT_TIMEOUT) | ||
| if (ret == WH_ERROR_OK) { | ||
| ret = wh_Client_InitCryptTimeout(ctx); | ||
| } | ||
| #endif | ||
| if (ret == 0) { | ||
| uint16_t resLen = 0; | ||
| do { | ||
| ret = | ||
| wh_Client_RecvResponse(ctx, &group, &action, &resLen, dataPtr); | ||
| _wait_response_with_crypttimeout(ctx, &group, &action, | ||
| &resLen, dataPtr); | ||
| } while (ret == WH_ERROR_NOTREADY); | ||
|
|
||
| if (ret == WH_ERROR_OK) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
usto simplify all of this.