Ensure that operation is put into error state if error occurs
If an error occurs, calling any function on the same operation should return
PSA_ERROR_BAD_STATE, and we were not honouring that for all errors. Add extra
failure tests to try and ratify this.
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
diff --git a/include/psa/crypto_struct.h b/include/psa/crypto_struct.h
index 1153b8e..934bc17 100644
--- a/include/psa/crypto_struct.h
+++ b/include/psa/crypto_struct.h
@@ -505,10 +505,12 @@
psa_driver_sign_hash_interruptible_context_t MBEDTLS_PRIVATE(ctx);
+ unsigned int MBEDTLS_PRIVATE(error_occurred) : 1;
+
uint32_t MBEDTLS_PRIVATE(num_ops);
};
-#define PSA_SIGN_HASH_INTERRUPTIBLE_OPERATION_INIT { 0, { 0 }, 0 }
+#define PSA_SIGN_HASH_INTERRUPTIBLE_OPERATION_INIT { 0, { 0 }, 0, 0 }
static inline struct psa_sign_hash_interruptible_operation_s
psa_sign_hash_interruptible_operation_init(void)
@@ -533,10 +535,12 @@
psa_driver_verify_hash_interruptible_context_t MBEDTLS_PRIVATE(ctx);
+ unsigned int MBEDTLS_PRIVATE(error_occurred) : 1;
+
uint32_t MBEDTLS_PRIVATE(num_ops);
};
-#define PSA_VERIFY_HASH_INTERRUPTIBLE_OPERATION_INIT { 0, { 0 }, 0 }
+#define PSA_VERIFY_HASH_INTERRUPTIBLE_OPERATION_INIT { 0, { 0 }, 0, 0 }
static inline struct psa_verify_hash_interruptible_operation_s
psa_verify_hash_interruptible_operation_init(void)
diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index a3bc806..882cb96 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -3183,14 +3183,15 @@
psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED;
psa_key_slot_t *slot;
- /* Check that start has not been previously called. */
- if (operation->id != 0) {
+ /* Check that start has not been previously called, or operation has not
+ * previously errored. */
+ if (operation->id != 0 || operation->error_occurred) {
return PSA_ERROR_BAD_STATE;
}
-
status = psa_sign_verify_check_alg(0, alg);
if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
return status;
}
@@ -3221,13 +3222,17 @@
exit:
if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
psa_sign_hash_abort_internal(operation);
}
unlock_status = psa_unlock_key_slot(slot);
- return (status == PSA_SUCCESS) ? unlock_status : status;
+ if (unlock_status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
+ }
+ return (status == PSA_SUCCESS) ? unlock_status : status;
}
@@ -3240,8 +3245,9 @@
*signature_length = 0;
- /* Check that start has been called first. */
- if (operation->id == 0) {
+ /* Check that start has been called first, and that operation has not
+ * previously errored. */
+ if (operation->id == 0 || operation->error_occurred) {
status = PSA_ERROR_BAD_STATE;
goto exit;
}
@@ -3276,6 +3282,10 @@
/* If signature_size is 0 then we have nothing to do. We must not
* call memset because signature may be NULL in this case.*/
+ if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
+ }
+
psa_sign_hash_abort_internal(operation);
}
@@ -3293,6 +3303,9 @@
* the operation fails or succeeds, only on manual abort. */
operation->num_ops = 0;
+ /* Likewise, failure state. */
+ operation->error_occurred = 0;
+
return status;
}
@@ -3325,13 +3338,15 @@
psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED;
psa_key_slot_t *slot;
- /* Check that start has not been previously called. */
- if (operation->id != 0) {
+ /* Check that start has not been previously called, or operation has not
+ * previously errored. */
+ if (operation->id != 0 || operation->error_occurred) {
return PSA_ERROR_BAD_STATE;
}
status = psa_sign_verify_check_alg(0, alg);
if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
return status;
}
@@ -3340,6 +3355,7 @@
alg);
if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
return status;
}
@@ -3357,14 +3373,17 @@
signature, signature_length);
if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
psa_verify_hash_abort_internal(operation);
}
unlock_status = psa_unlock_key_slot(slot);
- return (status == PSA_SUCCESS) ? unlock_status : status;
+ if (unlock_status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
+ }
- return status;
+ return (status == PSA_SUCCESS) ? unlock_status : status;
}
psa_status_t psa_verify_hash_complete(
@@ -3372,8 +3391,9 @@
{
psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
- /* Check that start has been called first. */
- if (operation->id == 0) {
+ /* Check that start has been called first, and that operation has not
+ * previously errored. */
+ if (operation->id == 0 || operation->error_occurred) {
status = PSA_ERROR_BAD_STATE;
goto exit;
}
@@ -3387,6 +3407,10 @@
operation);
if (status != PSA_OPERATION_INCOMPLETE) {
+ if (status != PSA_SUCCESS) {
+ operation->error_occurred = 1;
+ }
+
psa_verify_hash_abort_internal(operation);
}
@@ -3404,6 +3428,9 @@
* the operation fails or succeeds, only on manual abort. */
operation->num_ops = 0;
+ /* Likewise, failure state. */
+ operation->error_occurred = 0;
+
return status;
}
diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function
index f050abf..bee9232 100644
--- a/tests/suites/test_suite_psa_crypto.function
+++ b/tests/suites/test_suite_psa_crypto.function
@@ -6659,6 +6659,13 @@
TEST_EQUAL(actual_status, expected_start_status);
+ if (expected_start_status != PSA_SUCCESS) {
+ actual_status = psa_sign_hash_start(&operation, key, alg,
+ input_data->x, input_data->len);
+
+ TEST_EQUAL(actual_status, PSA_ERROR_BAD_STATE);
+ }
+
num_ops_prior = psa_sign_hash_get_num_ops(&operation);
TEST_ASSERT(num_ops_prior == 0);
@@ -6679,12 +6686,14 @@
}
} while (actual_status == PSA_OPERATION_INCOMPLETE);
- /* If the psa_sign_hash_start() failed, psa_sign_hash_complete()
- * should also fail with bad state. */
- if (expected_start_status != PSA_SUCCESS) {
+ TEST_EQUAL(actual_status, expected_complete_status);
+
+ if (expected_complete_status != PSA_SUCCESS) {
+ actual_status = psa_sign_hash_complete(&operation, signature,
+ signature_size,
+ &signature_length);
+
TEST_EQUAL(actual_status, PSA_ERROR_BAD_STATE);
- } else if (actual_status != PSA_OPERATION_INCOMPLETE) {
- TEST_EQUAL(actual_status, expected_complete_status);
}
PSA_ASSERT(psa_sign_hash_abort(&operation));
@@ -7121,6 +7130,15 @@
TEST_EQUAL(actual_status, expected_start_status);
+ if (expected_start_status != PSA_SUCCESS) {
+ actual_status = psa_verify_hash_start(&operation, key, alg,
+ hash_data->x, hash_data->len,
+ signature_data->x,
+ signature_data->len);
+
+ TEST_EQUAL(actual_status, PSA_ERROR_BAD_STATE);
+ }
+
num_ops_prior = psa_verify_hash_get_num_ops(&operation);
TEST_ASSERT(num_ops_prior == 0);
@@ -7139,12 +7157,12 @@
}
} while (actual_status == PSA_OPERATION_INCOMPLETE);
- /* If the psa_verify_hash_start() failed,
- * psa_verify_hash_complete() should also fail with bad state.*/
- if (expected_start_status != PSA_SUCCESS) {
+ TEST_EQUAL(actual_status, expected_complete_status);
+
+ if (expected_complete_status != PSA_SUCCESS) {
+ actual_status = psa_verify_hash_complete(&operation);
+
TEST_EQUAL(actual_status, PSA_ERROR_BAD_STATE);
- } else if (actual_status != PSA_OPERATION_INCOMPLETE) {
- TEST_EQUAL(actual_status, expected_complete_status);
}
TEST_LE_U(min_completes, num_completes);
@@ -7350,6 +7368,12 @@
&signature_length),
PSA_ERROR_BUFFER_TOO_SMALL);
+ /* And test that this invalidates the operation. */
+ TEST_EQUAL(psa_sign_hash_complete(&sign_operation, signature,
+ 0,
+ &signature_length),
+ PSA_ERROR_BAD_STATE);
+
PSA_ASSERT(psa_sign_hash_abort(&sign_operation));
/* Trash the hash buffer in between start and complete, to ensure