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>

include/psa/crypto_struct.h

@@ -505,10 +505,12 @@ struct psa_sign_hash_interruptible_operation_s {
 
     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 @@ struct psa_verify_hash_interruptible_operation_s {
 
     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)

library/psa_crypto.c

@@ -3183,14 +3183,15 @@ psa_status_t psa_sign_hash_start(
     psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED;
     psa_key_slot_t *slot;
 
-    /* Check that start has not been previously called. */
+    /* Check that start has not been previously called, or operation has not
-    if (operation->id != 0) {
+     * 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 @@ psa_status_t psa_sign_hash_start(
 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 @@ psa_status_t psa_sign_hash_complete(
 
     *signature_length = 0;
 
-    /* Check that start has been called first. */
+    /* Check that start has been called first, and that operation has not
-    if (operation->id == 0) {
+     * previously errored. */
+    if (operation->id == 0 || operation->error_occurred) {
         status = PSA_ERROR_BAD_STATE;
         goto exit;
     }
@@ -3276,6 +3282,10 @@ exit:
         /* 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 @@ psa_status_t psa_sign_hash_abort(
      * 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 psa_verify_hash_start(
     psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED;
     psa_key_slot_t *slot;
 
-    /* Check that start has not been previously called. */
+    /* Check that start has not been previously called, or operation has not
-    if (operation->id != 0) {
+     * 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 @@ psa_status_t psa_verify_hash_start(
                                                    alg);
 
     if (status != PSA_SUCCESS) {
+        operation->error_occurred = 1;
         return status;
     }
 
@@ -3357,14 +3373,17 @@ psa_status_t psa_verify_hash_start(
                                                   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 psa_verify_hash_complete(
 {
     psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
 
-    /* Check that start has been called first. */
+    /* Check that start has been called first, and that operation has not
-    if (operation->id == 0) {
+     * previously errored. */
+    if (operation->id == 0 || operation->error_occurred) {
         status = PSA_ERROR_BAD_STATE;
         goto exit;
     }
@@ -3387,6 +3407,10 @@ exit:
         operation);
 
     if (status != PSA_OPERATION_INCOMPLETE) {
+        if (status != PSA_SUCCESS) {
+            operation->error_occurred = 1;
+        }
+
         psa_verify_hash_abort_internal(operation);
     }
 
@@ -3404,6 +3428,9 @@ psa_status_t psa_verify_hash_abort(
      * the operation fails or succeeds, only on manual abort. */
     operation->num_ops = 0;
 
+    /* Likewise, failure state. */
+    operation->error_occurred = 0;
+
     return status;
 }
 

tests/suites/test_suite_psa_crypto.function

@@ -6659,6 +6659,13 @@ void sign_hash_fail_interruptible(int key_type_arg, data_t *key_data,
 
     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 @@ void sign_hash_fail_interruptible(int key_type_arg, data_t *key_data,
         }
     } 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, PSA_ERROR_BAD_STATE);
-    } else if (actual_status != PSA_OPERATION_INCOMPLETE) {
     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);
     }
 
     PSA_ASSERT(psa_sign_hash_abort(&operation));
@@ -7121,6 +7130,15 @@ void verify_hash_fail_interruptible(int key_type_arg, data_t *key_data,
 
     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 @@ void verify_hash_fail_interruptible(int key_type_arg, data_t *key_data,
         }
     } 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, PSA_ERROR_BAD_STATE);
-    } else if (actual_status != PSA_OPERATION_INCOMPLETE) {
     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);
     }
 
     TEST_LE_U(min_completes, num_completes);
@@ -7350,6 +7368,12 @@ void hash_interruptible_state_test(int key_type_arg, data_t *key_data,
                                       &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